From 01355325d96521e20e6288e1756bd9372a0fa031 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 5 Oct 2022 15:27:47 +0800 Subject: [PATCH 01/42] checkboxes for upcoming bare clone capability --- README.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 0e66e28b1f4..3620566c01d 100644 --- a/README.md +++ b/README.md @@ -52,7 +52,10 @@ Please see _'Development Status'_ for a listing of all crates and their capabili * **remote** * [x] **refs** - list all references available on the remote based on the current remote configuration. * [x] **ref-map** - show how remote references relate to their local tracking branches as mapped by refspecs. - * **fetch** - fetch the current remote or the given one, optionally just as dry-run. + * [x] **fetch** - fetch the current remote or the given one, optionally just as dry-run. + * **clone** + * [ ] initialize a new **bare** repository and fetch all objects. + * [ ] initialize a new repository, fetch all objects and checkout the main worktree. * **credential** * [x] **fill/approve/reject** - The same as `git credential`, but implemented in Rust, calling helpers only when from trusted configuration. * **free** - no git repository necessary From 50fbcca85fdf02261fba954363bcf257cdf445fe Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 5 Oct 2022 18:07:27 +0800 Subject: [PATCH 02/42] layout save API for remotes --- git-repository/src/remote/mod.rs | 3 +++ git-repository/src/remote/save.rs | 43 +++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+) create mode 100644 git-repository/src/remote/save.rs diff --git a/git-repository/src/remote/mod.rs b/git-repository/src/remote/mod.rs index 661ed9e5910..fadd402fef0 100644 --- a/git-repository/src/remote/mod.rs +++ b/git-repository/src/remote/mod.rs @@ -38,5 +38,8 @@ mod connection; #[cfg(any(feature = "async-network-client", feature = "blocking-network-client"))] pub use connection::{ref_map, Connection}; +/// +pub mod save; + mod access; pub(crate) mod url; diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs new file mode 100644 index 00000000000..8f5fdd6258b --- /dev/null +++ b/git-repository/src/remote/save.rs @@ -0,0 +1,43 @@ +use crate::Remote; + +/// The error returned by [`Remote::save_to()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("The remote pointing to {} is anonymous and can't be saved.", url.to_bstring())] + NameMissing { url: git_url::Url }, +} + +/// Serialize into git-config. +impl Remote<'_> { + /// Save ourselves to the given `config` if we are a named remote or fail otherwise. + /// + /// Note that all sections named `remote ""` will be cleared of all values we are about to write, + /// and the last `remote ""` section will be containing all relevant values so that reloading the remote + /// from `config` would yield the same in-memory state. + pub fn save_to(&self, _config: &mut git_config::File<'static>) -> Result<(), Error> { + let _name = self.name().ok_or_else(|| Error::NameMissing { + url: self + .url + .as_ref() + .or_else(|| self.push_url.as_ref()) + .expect("one url is always set") + .to_owned(), + })?; + todo!() + } + + /// Forcefully set our name to `name` and write our state to `config` similar to [`save_to()`][Self::save_to()]. + /// + /// Note that this sets a name for anonymous remotes, but overwrites the name for those who were named before. + /// If this name is different from the current one, the git configuration will still contain the previous name, + /// and the caller should account for that. + pub fn save_as( + &mut self, + name: git_config::parse::section::Name<'_>, + config: &mut git_config::File<'static>, + ) -> Result<(), Error> { + self.name = Some(name.to_string()); + self.save_to(config) + } +} From 1cba50f908ea016c9117e2de2181f55326f14e7e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 5 Oct 2022 18:08:12 +0800 Subject: [PATCH 03/42] thanks clippy --- git-repository/src/remote/save.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index 8f5fdd6258b..9870c90eab1 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -20,7 +20,7 @@ impl Remote<'_> { url: self .url .as_ref() - .or_else(|| self.push_url.as_ref()) + .or(self.push_url.as_ref()) .expect("one url is always set") .to_owned(), })?; From e1f7c5f05095b6b3d2d26c34fda04b74a91c9fb0 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 16:11:08 +0800 Subject: [PATCH 04/42] A basic save_to() implementation for url and push-url For now without any handling of removing previous values. --- git-repository/src/remote/save.rs | 24 ++++++++++++++++----- git-repository/tests/remote/mod.rs | 1 + git-repository/tests/remote/save.rs | 33 +++++++++++++++++++++++++++++ 3 files changed, 53 insertions(+), 5 deletions(-) create mode 100644 git-repository/tests/remote/save.rs diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index 9870c90eab1..34c25cfab85 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -1,4 +1,5 @@ use crate::Remote; +use std::convert::TryInto; /// The error returned by [`Remote::save_to()`]. #[derive(Debug, thiserror::Error)] @@ -15,8 +16,8 @@ impl Remote<'_> { /// Note that all sections named `remote ""` will be cleared of all values we are about to write, /// and the last `remote ""` section will be containing all relevant values so that reloading the remote /// from `config` would yield the same in-memory state. - pub fn save_to(&self, _config: &mut git_config::File<'static>) -> Result<(), Error> { - let _name = self.name().ok_or_else(|| Error::NameMissing { + pub fn save_to(&self, config: &mut git_config::File<'static>) -> Result<(), Error> { + let name = self.name().ok_or_else(|| Error::NameMissing { url: self .url .as_ref() @@ -24,7 +25,16 @@ impl Remote<'_> { .expect("one url is always set") .to_owned(), })?; - todo!() + let mut section = config + .section_mut_or_create_new("remote", Some(name)) + .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())) + } + if let Some(url) = self.push_url.as_ref() { + section.push("pushurl".try_into().expect("valid"), Some(url.to_bstring().as_ref())) + } + Ok(()) } /// Forcefully set our name to `name` and write our state to `config` similar to [`save_to()`][Self::save_to()]. @@ -32,12 +42,16 @@ impl Remote<'_> { /// Note that this sets a name for anonymous remotes, but overwrites the name for those who were named before. /// If this name is different from the current one, the git configuration will still contain the previous name, /// and the caller should account for that. - pub fn save_as( + pub fn save_as_to( &mut self, name: git_config::parse::section::Name<'_>, config: &mut git_config::File<'static>, ) -> Result<(), Error> { + let prev_name = self.name.take(); self.name = Some(name.to_string()); - self.save_to(config) + self.save_to(config).map_err(|err| { + self.name = prev_name; + err + }) } } diff --git a/git-repository/tests/remote/mod.rs b/git-repository/tests/remote/mod.rs index ea4b5c03e75..b634044ef5b 100644 --- a/git-repository/tests/remote/mod.rs +++ b/git-repository/tests/remote/mod.rs @@ -19,3 +19,4 @@ pub(crate) fn cow_str(s: &str) -> Cow { mod connect; mod fetch; mod ref_map; +mod save; diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs new file mode 100644 index 00000000000..0be529317ef --- /dev/null +++ b/git-repository/tests/remote/save.rs @@ -0,0 +1,33 @@ +mod save_as_to { + use crate::basic_repo; + use git_repository as git; + use std::convert::TryInto; + + #[test] + fn new_anonymous_remote_with_name() { + let repo = basic_repo().unwrap(); + let mut remote = repo + .remote_at("https://example.com/path") + .unwrap() + .push_url("https://ein.hub/path") + .unwrap(); + let remote_name = "origin"; + assert!( + repo.find_remote(remote_name).is_err(), + "there is no remote of that name" + ); + assert_eq!(remote.name(), None); + let mut config = git::config::File::default(); + remote + .save_as_to(remote_name.try_into().expect("valid name"), &mut config) + .unwrap(); + assert_eq!( + uniformize(config.to_string()), + "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n" + ) + } + + fn uniformize(input: String) -> String { + input.replace("\r\n", "\n") + } +} From 769e89790d8c4146263c84d4d5c9dff7d5018919 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 16:15:37 +0800 Subject: [PATCH 05/42] improve clarity docs related to mutating sections --- git-config/src/file/access/mutate.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/git-config/src/file/access/mutate.rs b/git-config/src/file/access/mutate.rs index b70930eca1d..e6ef43cbf66 100644 --- a/git-config/src/file/access/mutate.rs +++ b/git-config/src/file/access/mutate.rs @@ -11,7 +11,7 @@ use crate::{ /// Mutating low-level access methods. impl<'event> File<'event> { - /// Returns an mutable section with a given `name` and optional `subsection_name`, _if it exists_. + /// Returns the last mutable section with a given `name` and optional `subsection_name`, _if it exists_. pub fn section_mut<'a>( &'a mut self, name: impl AsRef, @@ -29,7 +29,7 @@ impl<'event> File<'event> { .expect("BUG: Section did not have id from lookup") .to_mut(nl)) } - /// Returns an mutable section with a given `name` and optional `subsection_name`, _if it exists_, or create a new section. + /// Returns the last mutable section with a given `name` and optional `subsection_name`, _if it exists_, or create a new section. pub fn section_mut_or_create_new<'a>( &'a mut self, name: impl AsRef, From 1375368006942bdf246a85466a144b05df5a55ac Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 16:16:00 +0800 Subject: [PATCH 06/42] test another prerequisite --- git-repository/tests/remote/save.rs | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 0be529317ef..acc2c522a51 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -4,13 +4,22 @@ mod save_as_to { use std::convert::TryInto; #[test] - fn new_anonymous_remote_with_name() { - let repo = basic_repo().unwrap(); + fn anonymous_remotes_cannot_be_save_lacking_a_name() -> crate::Result { + let repo = basic_repo()?; + let remote = repo.remote_at("https://example.com/path")?; + assert!(matches!( + remote.save_to(&mut git::config::File::default()).unwrap_err(), + git::remote::save::Error::NameMissing { .. } + )); + Ok(()) + } + + #[test] + fn new_anonymous_remote_with_name() -> crate::Result { + let repo = basic_repo()?; let mut remote = repo - .remote_at("https://example.com/path") - .unwrap() - .push_url("https://ein.hub/path") - .unwrap(); + .remote_at("https://example.com/path")? + .push_url("https://ein.hub/path")?; let remote_name = "origin"; assert!( repo.find_remote(remote_name).is_err(), @@ -18,13 +27,12 @@ mod save_as_to { ); assert_eq!(remote.name(), None); let mut config = git::config::File::default(); - remote - .save_as_to(remote_name.try_into().expect("valid name"), &mut config) - .unwrap(); + remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; assert_eq!( uniformize(config.to_string()), "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n" - ) + ); + Ok(()) } fn uniformize(input: String) -> String { From 79a495249ded5dbab0084283606d8775911dbfca Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 16:32:24 +0800 Subject: [PATCH 07/42] save all data we currently use on a Remote --- git-repository/src/remote/save.rs | 11 +++++++++++ git-repository/tests/remote/save.rs | 11 +++++++++-- 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index 34c25cfab85..c2299343d57 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -34,6 +34,17 @@ impl Remote<'_> { if let Some(url) = self.push_url.as_ref() { section.push("pushurl".try_into().expect("valid"), Some(url.to_bstring().as_ref())) } + for (key, spec) in self + .fetch_specs + .iter() + .map(|spec| ("fetch", spec)) + .chain(self.push_specs.iter().map(|spec| ("push", spec))) + { + section.push( + key.try_into().expect("valid"), + Some(spec.to_ref().to_bstring().as_ref()), + ) + } Ok(()) } diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index acc2c522a51..7a8cd54df25 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -19,7 +19,14 @@ mod save_as_to { let repo = basic_repo()?; let mut remote = repo .remote_at("https://example.com/path")? - .push_url("https://ein.hub/path")?; + .push_url("https://ein.hub/path")? + .with_refspec("+refs/heads/*:refs/remotes/any/*", git::remote::Direction::Fetch)? + .with_refspec( + "refs/heads/special:refs/heads/special-upstream", + git::remote::Direction::Fetch, + )? + .with_refspec("refs/heads/main:refs/heads/main", git::remote::Direction::Push)? // similar to 'simple' for `push.default` + .with_refspec(":", git::remote::Direction::Push)?; // similar to 'matching' let remote_name = "origin"; assert!( repo.find_remote(remote_name).is_err(), @@ -30,7 +37,7 @@ mod save_as_to { remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; assert_eq!( uniformize(config.to_string()), - "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n" + "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n" ); Ok(()) } From e533993e8f861ba7a6600aab114ddfecc8a85ee2 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 19:59:47 +0800 Subject: [PATCH 08/42] fix: `File::remove_section()` was fixed to allow re-adding a similarly named section. We also add `File::remove_section_by_id()` to make it possible to remove specific sections. --- git-config/src/file/access/mutate.rs | 41 +++++++++++++++++++++----- git-config/src/file/mod.rs | 2 +- git-config/tests/file/access/mutate.rs | 23 +++++++++++++++ 3 files changed, 58 insertions(+), 8 deletions(-) diff --git a/git-config/src/file/access/mutate.rs b/git-config/src/file/access/mutate.rs index e6ef43cbf66..b7cf3a3dc58 100644 --- a/git-config/src/file/access/mutate.rs +++ b/git-config/src/file/access/mutate.rs @@ -2,6 +2,7 @@ use std::borrow::Cow; use git_features::threading::OwnShared; +use crate::file::SectionBodyIdsLut; use crate::{ file::{self, rename_section, write::ends_with_newline, MetadataFilter, SectionId, SectionMut}, lookup, @@ -182,13 +183,39 @@ impl<'event> File<'event> { .ok()? .rev() .next()?; - self.section_order.remove( - self.section_order - .iter() - .position(|v| *v == id) - .expect("known section id"), - ); - self.sections.remove(&id) + self.remove_section_by_id(id) + } + + /// Remove the section identified by `id` if it exists and return it, or return `None` if no such section was present. + /// + /// Note that section ids are unambiguous even in the face of removals and additions of sections. + pub fn remove_section_by_id(&mut self, id: SectionId) -> Option> { + self.section_order + .remove(self.section_order.iter().position(|v| *v == id)?); + let section = self.sections.remove(&id)?; + let lut = self + .section_lookup_tree + .get_mut(§ion.header.name) + .expect("lookup cache still has name to be deleted"); + for entry in lut { + match section.header.subsection_name.as_deref() { + Some(subsection_name) => { + if let SectionBodyIdsLut::NonTerminal(map) = entry { + if let Some(ids) = map.get_mut(subsection_name) { + ids.remove(ids.iter().position(|v| *v == id).expect("present")); + break; + } + } + } + None => { + if let SectionBodyIdsLut::Terminal(ids) = entry { + ids.remove(ids.iter().position(|v| *v == id).expect("present")); + break; + } + } + } + } + Some(section) } /// Removes the section with `name` and `subsection_name` that passed `filter`, returning the removed section diff --git a/git-config/src/file/mod.rs b/git-config/src/file/mod.rs index e799b79b494..16209772ead 100644 --- a/git-config/src/file/mod.rs +++ b/git-config/src/file/mod.rs @@ -110,7 +110,7 @@ impl AddAssign for Size { /// words, it's possible that a section may have an ID of 3 but the next section /// has an ID of 5 as 4 was deleted. #[derive(PartialEq, Eq, Hash, Copy, Clone, PartialOrd, Ord, Debug)] -pub(crate) struct SectionId(pub(crate) usize); +pub struct SectionId(pub(crate) usize); /// All section body ids referred to by a section name. /// diff --git a/git-config/tests/file/access/mutate.rs b/git-config/tests/file/access/mutate.rs index de28bf9c641..d3834bc742f 100644 --- a/git-config/tests/file/access/mutate.rs +++ b/git-config/tests/file/access/mutate.rs @@ -1,3 +1,26 @@ +mod remove_section { + use std::convert::TryFrom; + + #[test] + fn removal_is_complete() { + let mut file = git_config::File::try_from("[core] \na = b\nb=c\n\n[core \"name\"]\nd = 1\ne = 2").unwrap(); + assert_eq!(file.sections().count(), 2); + + let removed = file.remove_section("core", None).expect("removed correct section"); + assert_eq!(removed.header().name(), "core"); + assert_eq!(removed.header().subsection_name(), None); + assert_eq!(file.sections().count(), 1); + + let removed = file.remove_section("core", Some("name")).expect("found"); + assert_eq!(removed.header().name(), "core"); + assert_eq!(removed.header().subsection_name().expect("present"), "name"); + assert_eq!(file.sections().count(), 0); + + file.section_mut_or_create_new("core", None).expect("creation succeeds"); + file.section_mut_or_create_new("core", Some("name")) + .expect("creation succeeds"); + } +} mod rename_section { use std::{borrow::Cow, convert::TryFrom}; From 5df2a2a5a9addbda7dcc68b2f8f7f4a48d9720c6 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 20:31:56 +0800 Subject: [PATCH 09/42] feat: Add various methods to iterate sections along with their id, and mutate them. As section names are not unique, it was previously not possible to iterate sections and then mutate them as one wouldn't be able to refer to the exact section that was just traversed, after all, there can be many sections named `remote "origin"`. With the new methods it's possible to uniquely refer to each section for mutation and removal. --- git-config/src/file/access/mutate.rs | 9 ++++++++ git-config/src/file/access/read_only.rs | 29 ++++++++++++++++++++++-- git-config/tests/file/access/mutate.rs | 27 +++++++++++++++++++++- git-config/tests/file/mutable/section.rs | 9 ++++++++ 4 files changed, 71 insertions(+), 3 deletions(-) diff --git a/git-config/src/file/access/mutate.rs b/git-config/src/file/access/mutate.rs index b7cf3a3dc58..ff8a88e8b95 100644 --- a/git-config/src/file/access/mutate.rs +++ b/git-config/src/file/access/mutate.rs @@ -30,6 +30,15 @@ impl<'event> File<'event> { .expect("BUG: Section did not have id from lookup") .to_mut(nl)) } + + /// Return the mutable section identified by `id`, or `None` if it didn't exist. + /// + /// Note that `id` is stable across deletions and insertions. + pub fn section_mut_by_id<'a>(&'a mut self, id: SectionId) -> Option> { + let nl = self.detect_newline_style_smallvec(); + self.sections.get_mut(&id).map(|s| s.to_mut(nl)) + } + /// Returns the last mutable section with a given `name` and optional `subsection_name`, _if it exists_, or create a new section. pub fn section_mut_or_create_new<'a>( &'a mut self, diff --git a/git-config/src/file/access/read_only.rs b/git-config/src/file/access/read_only.rs index 56e274d7b05..a50d60137f2 100644 --- a/git-config/src/file/access/read_only.rs +++ b/git-config/src/file/access/read_only.rs @@ -1,9 +1,10 @@ -use std::{borrow::Cow, convert::TryFrom, iter::FromIterator}; +use std::{borrow::Cow, convert::TryFrom}; use bstr::BStr; use git_features::threading::OwnShared; use smallvec::SmallVec; +use crate::file::SectionId; use crate::{ file, file::{ @@ -206,6 +207,25 @@ impl<'event> File<'event> { }) } + /// Similar to [`sections_by_name()`][Self::sections_by_name()], but returns an identifier for this section as well to allow + /// referring to it unambiguously even in the light of deletions. + #[must_use] + pub fn sections_and_ids_by_name<'a>( + &'a self, + name: &'a str, + ) -> Option, SectionId)> + '_> { + self.section_ids_by_name(name).ok().map(move |ids| { + ids.map(move |id| { + ( + self.sections + .get(&id) + .expect("section doesn't have id from from lookup"), + id, + ) + }) + }) + } + /// Gets all sections that match the provided `name`, ignoring any subsections, and pass the `filter`. #[must_use] pub fn sections_by_name_and_filter<'a>( @@ -258,6 +278,11 @@ impl<'event> File<'event> { self.section_order.iter().map(move |id| &self.sections[id]) } + /// Return an iterator over all sections and their ids, in order of occurrence in the file itself. + pub fn sections_and_ids(&self) -> impl Iterator, SectionId)> + '_ { + self.section_order.iter().map(move |id| (&self.sections[id], *id)) + } + /// Return an iterator over all sections along with non-section events that are placed right after them, /// in order of occurrence in the file itself. /// @@ -296,6 +321,6 @@ impl<'event> File<'event> { } pub(crate) fn detect_newline_style_smallvec(&self) -> SmallVec<[u8; 2]> { - SmallVec::from_iter(self.detect_newline_style().iter().copied()) + self.detect_newline_style().as_ref().into() } } diff --git a/git-config/tests/file/access/mutate.rs b/git-config/tests/file/access/mutate.rs index d3834bc742f..6fb90ee693d 100644 --- a/git-config/tests/file/access/mutate.rs +++ b/git-config/tests/file/access/mutate.rs @@ -2,7 +2,32 @@ mod remove_section { use std::convert::TryFrom; #[test] - fn removal_is_complete() { + fn removal_of_all_sections_programmatically_with_sections_and_ids_by_name() { + let mut file = git_config::File::try_from("[core] \na = b\nb=c\n\n[core \"name\"]\nd = 1\ne = 2").unwrap(); + for id in file + .sections_and_ids_by_name("core") + .expect("2 sections present") + .map(|(_, id)| id) + .collect::>() + { + file.remove_section_by_id(id); + } + assert!(file.is_void()); + assert_eq!(file.sections().count(), 0); + } + + #[test] + fn removal_of_all_sections_programmatically_with_sections_and_ids() { + let mut file = git_config::File::try_from("[core] \na = b\nb=c\n\n[core \"name\"]\nd = 1\ne = 2").unwrap(); + for id in file.sections_and_ids().map(|(_, id)| id).collect::>() { + file.remove_section_by_id(id); + } + assert!(file.is_void()); + assert_eq!(file.sections().count(), 0); + } + + #[test] + fn removal_is_complete_and_sections_can_be_readded() { let mut file = git_config::File::try_from("[core] \na = b\nb=c\n\n[core \"name\"]\nd = 1\ne = 2").unwrap(); assert_eq!(file.sections().count(), 2); diff --git a/git-config/tests/file/mutable/section.rs b/git-config/tests/file/mutable/section.rs index 0f878bb8fde..02d89c3d681 100644 --- a/git-config/tests/file/mutable/section.rs +++ b/git-config/tests/file/mutable/section.rs @@ -28,6 +28,15 @@ fn section_mut_or_create_new_filter_may_reject_existing_sections() -> crate::Res Ok(()) } +#[test] +fn section_mut_by_id() { + let mut config = multi_value_section(); + let id = config.sections_and_ids().next().expect("at least one").1; + let section = config.section_mut_by_id(id).expect("present"); + assert_eq!(section.header().name(), "a"); + assert_eq!(section.header().subsection_name(), None); +} + mod remove { use super::multi_value_section; From 4126d996b5c110e2a73f4b94f7166d7e66e059b3 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 20:44:05 +0800 Subject: [PATCH 10/42] remove prior existing values for remotes before saving --- git-repository/src/remote/save.rs | 23 +++++++++++++++++++++++ git-repository/tests/remote/save.rs | 11 ++++++++++- 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index c2299343d57..46cf131ea86 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -25,6 +25,29 @@ impl Remote<'_> { .expect("one url is always set") .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)) + .collect::>() + }) { + let mut sections_to_remove = Vec::new(); + const KEYS_TO_REMOVE: &[&'static str] = &["url", "pushurl", "fetch", "push"]; + for id in section_ids { + let mut section = config.section_mut_by_id(id).expect("just queried"); + let was_empty = section.num_values() == 0; + + for key in KEYS_TO_REMOVE { + while let Some(_) = section.remove(key) {} + } + + let is_empty_after_deletions_of_values_to_be_written = section.num_values() == 0; + if !was_empty && is_empty_after_deletions_of_values_to_be_written { + sections_to_remove.push(id); + } + } + for id in sections_to_remove { + config.remove_section_by_id(id); + } + } let mut section = config .section_mut_or_create_new("remote", Some(name)) .expect("section name is validated and 'remote' is acceptable"); diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 7a8cd54df25..c79016e372a 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -34,14 +34,23 @@ mod save_as_to { ); assert_eq!(remote.name(), None); let mut config = git::config::File::default(); + remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; + let expected = "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n"; + assert_eq!(uniformize(config.to_string()), expected); + remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; assert_eq!( uniformize(config.to_string()), - "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n" + expected, + "it appears to be idempotent in this case" ); Ok(()) } + #[test] + #[ignore] + fn save_existing_and_leave_extra_values_we_do_not_touch() {} + fn uniformize(input: String) -> String { input.replace("\r\n", "\n") } From d82a08db71f6a486363a398eacdee6cdfbbeb04b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 20:44:58 +0800 Subject: [PATCH 11/42] thanks clippy --- etc/check-package-size.sh | 2 +- git-repository/src/remote/save.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/etc/check-package-size.sh b/etc/check-package-size.sh index 3770e5f9110..17b6eb74163 100755 --- a/etc/check-package-size.sh +++ b/etc/check-package-size.sh @@ -57,6 +57,6 @@ echo "in root: gitoxide CLI" (enter git-odb && indent cargo diet -n --package-size-limit 120KB) (enter git-protocol && indent cargo diet -n --package-size-limit 50KB) (enter git-packetline && indent cargo diet -n --package-size-limit 35KB) -(enter git-repository && indent cargo diet -n --package-size-limit 185KB) +(enter git-repository && indent cargo diet -n --package-size-limit 200KB) (enter git-transport && indent cargo diet -n --package-size-limit 55KB) (enter gitoxide-core && indent cargo diet -n --package-size-limit 90KB) diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index 46cf131ea86..0d7838596d2 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -30,13 +30,13 @@ impl Remote<'_> { .collect::>() }) { let mut sections_to_remove = Vec::new(); - const KEYS_TO_REMOVE: &[&'static str] = &["url", "pushurl", "fetch", "push"]; + const KEYS_TO_REMOVE: &[&str] = &["url", "pushurl", "fetch", "push"]; for id in section_ids { let mut section = config.section_mut_by_id(id).expect("just queried"); let was_empty = section.num_values() == 0; for key in KEYS_TO_REMOVE { - while let Some(_) = section.remove(key) {} + while section.remove(key).is_some() {} } let is_empty_after_deletions_of_values_to_be_written = section.num_values() == 0; From ca5bb5ec44f098f74a47835ee0dbc694cbb76e0c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 21:50:42 +0800 Subject: [PATCH 12/42] assure unrelated and unwritten keys are not touched --- git-repository/tests/remote/save.rs | 24 ++++++++++++++++++++---- 1 file changed, 20 insertions(+), 4 deletions(-) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index c79016e372a..e9452b697ba 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -44,13 +44,29 @@ mod save_as_to { expected, "it appears to be idempotent in this case" ); + + { + let mut new_section = config.section_mut_or_create_new("unrelated", None).expect("works"); + new_section.push("a".try_into().unwrap(), Some("value".into())); + + config + .section_mut_or_create_new("initially-empty-not-removed", Some("name")) + .expect("works"); + + let mut existing_section = config + .section_mut_or_create_new("remote", Some("origin")) + .expect("works"); + existing_section.push("free".try_into().unwrap(), Some("should not be removed".into())) + } + remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; + assert_eq!( + uniformize(config.to_string()), + "[remote \"origin\"]\n\t\n\t\n\t\n\t\n\t\n\t\n\tfree = should not be removed\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n[unrelated]\n\ta = value\n[initially-empty-not-removed \"name\"]\n", + "unrelated keys are kept, and so are keys in the sections we edit" + ); Ok(()) } - #[test] - #[ignore] - fn save_existing_and_leave_extra_values_we_do_not_touch() {} - fn uniformize(input: String) -> String { input.replace("\r\n", "\n") } From 9c1e639979a9615fd8334ce0e3a809df137776f6 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 22:34:30 +0800 Subject: [PATCH 13/42] fix: greatly improve whitespace handling when removing values. Previously, newlines would remain past a value, and whitespace could remain before one. Now both are removed to simulate removing an actual line. --- git-config/src/file/mutable/section.rs | 34 +++++++++++++++++------- git-config/tests/file/mutable/section.rs | 5 +--- 2 files changed, 26 insertions(+), 13 deletions(-) diff --git a/git-config/src/file/mutable/section.rs b/git-config/src/file/mutable/section.rs index 38fd3ec6f1c..26b1abacc76 100644 --- a/git-config/src/file/mutable/section.rs +++ b/git-config/src/file/mutable/section.rs @@ -95,7 +95,7 @@ impl<'a, 'event> SectionMut<'a, 'event> { Some((key_range, value_range)) => { let value_range = value_range.unwrap_or(key_range.end - 1..key_range.end); let range_start = value_range.start; - let ret = self.remove_internal(value_range); + let ret = self.remove_internal(value_range, false); self.section .body .0 @@ -109,7 +109,7 @@ impl<'a, 'event> SectionMut<'a, 'event> { pub fn remove(&mut self, key: impl AsRef) -> Option> { let key = Key::from_str_unchecked(key.as_ref()); let (key_range, _value_range) = self.key_and_value_range_by(&key)?; - Some(self.remove_internal(key_range)) + Some(self.remove_internal(key_range, true)) } /// Adds a new line event. Note that you don't need to call this unless @@ -231,17 +231,33 @@ impl<'a, 'event> SectionMut<'a, 'event> { } /// Performs the removal, assuming the range is valid. - fn remove_internal(&mut self, range: Range) -> Cow<'event, BStr> { - self.section - .body - .0 - .drain(range) - .fold(Cow::Owned(BString::default()), |mut acc, e| { + fn remove_internal(&mut self, range: Range, fix_whitespace: bool) -> Cow<'event, BStr> { + let events = &mut self.section.body.0; + if fix_whitespace + && events + .get(range.end) + .map_or(false, |ev| matches!(ev, Event::Newline(_))) + { + events.remove(range.end); + } + let value = events + .drain(range.clone()) + .fold(Cow::Owned(BString::default()), |mut acc: Cow<'_, BStr>, e| { if let Event::Value(v) | Event::ValueNotDone(v) | Event::ValueDone(v) = e { acc.to_mut().extend(&**v); } acc - }) + }); + if fix_whitespace + && range + .start + .checked_sub(1) + .and_then(|pos| events.get(pos)) + .map_or(false, |ev| matches!(ev, Event::Whitespace(_))) + { + events.remove(range.start - 1); + } + value } } diff --git a/git-config/tests/file/mutable/section.rs b/git-config/tests/file/mutable/section.rs index 02d89c3d681..b8e890480d5 100644 --- a/git-config/tests/file/mutable/section.rs +++ b/git-config/tests/file/mutable/section.rs @@ -58,10 +58,7 @@ mod remove { } assert!(!section.is_void(), "everything is still there"); - assert_eq!( - config.to_string(), - "\n [a]\n \n \n \n \n " - ); + assert_eq!(config.to_string(), "\n [a]\n"); Ok(()) } } From 058473d5eae4789ca7b3fcb203ae6349a3e6d25a Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 8 Oct 2022 22:35:35 +0800 Subject: [PATCH 14/42] adapt to changes in `git-config` Much better whitespace handling definitely shows here. --- git-repository/tests/remote/save.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index e9452b697ba..2950274d45a 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -61,7 +61,7 @@ mod save_as_to { remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; assert_eq!( uniformize(config.to_string()), - "[remote \"origin\"]\n\t\n\t\n\t\n\t\n\t\n\t\n\tfree = should not be removed\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n[unrelated]\n\ta = value\n[initially-empty-not-removed \"name\"]\n", + "[remote \"origin\"]\n\tfree = should not be removed\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n[unrelated]\n\ta = value\n[initially-empty-not-removed \"name\"]\n", "unrelated keys are kept, and so are keys in the sections we edit" ); Ok(()) From 884b6e93962bf782bfca14873277a8c563b58d61 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 10:09:24 +0800 Subject: [PATCH 15/42] add test to assert on lossless saving of existing named remotes It also showed that we aren't quite lossless with our urls yet. --- git-repository/tests/remote/save.rs | 52 +++++++++++++++++++++++++++-- 1 file changed, 49 insertions(+), 3 deletions(-) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 2950274d45a..64381ed36e2 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -1,5 +1,51 @@ +mod save_to { + use crate::remote; + use crate::remote::save::uniformize; + use git_repository as git; + + #[test] + fn named_remotes_save_as_is() -> crate::Result { + let repo = remote::repo("clone"); + let remote = repo.find_remote("origin")?; + + let mut config = git::config::File::default(); + remote.save_to(&mut config)?; + let actual = uniformize(config.to_string()); + assert!( + actual.starts_with("[remote \"origin\"]\n\turl = "), + "workaround absolute paths in test fixture…" + ); + assert!( + actual.ends_with("/base\n\tfetch = +refs/heads/*:refs/remotes/origin/*\n"), + "…by checking only the parts that are similar" + ); + + // TODO: fix this + // let previous_remote_state = repo + // .config_snapshot() + // .plumbing() + // .section("remote", Some("origin")) + // .expect("present") + // .to_bstring(); + let mut config = repo.config_snapshot().plumbing().clone(); + remote.save_to(&mut config)?; + assert_eq!( + config.sections_by_name("remote").expect("more than one").count(), + 2, + "amount of remotes are unaltered" + ); + // assert_eq!( + // config.section("remote", Some("origin")).expect("present").to_bstring(), + // previous_remote_state, + // "the serialization doesn't modify anything" + // ); + Ok(()) + } +} + mod save_as_to { use crate::basic_repo; + use crate::remote::save::uniformize; use git_repository as git; use std::convert::TryInto; @@ -66,8 +112,8 @@ mod save_as_to { ); Ok(()) } +} - fn uniformize(input: String) -> String { - input.replace("\r\n", "\n") - } +fn uniformize(input: String) -> String { + input.replace("\r\n", "\n") } From 58a6000d669acd33bad91509eaa469f041f119e5 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 10:32:59 +0800 Subject: [PATCH 16/42] feat: lossless serialization of file urls. Previously a url like `/path/to/repo` would serialize to `file:///path/to/repo`, preventing round-trips. Now it serializes like it was parsed. This also means that `file://path` still serializes as `file://path`. --- git-url/src/impls.rs | 1 + git-url/src/lib.rs | 22 ++++++++++++-- git-url/src/parse.rs | 17 +++++------ git-url/tests/parse/file.rs | 58 ++++++++++++++++++++++++------------- git-url/tests/parse/mod.rs | 10 +++++++ 5 files changed, 77 insertions(+), 31 deletions(-) diff --git a/git-url/src/impls.rs b/git-url/src/impls.rs index 15881520515..1288cd2e2f1 100644 --- a/git-url/src/impls.rs +++ b/git-url/src/impls.rs @@ -7,6 +7,7 @@ use crate::{parse, Scheme, Url}; impl Default for Url { fn default() -> Self { Url { + serialize_alternative_form: false, scheme: Scheme::Ssh, user: None, host: None, diff --git a/git-url/src/lib.rs b/git-url/src/lib.rs index d47aa85942c..6da0d872fb3 100644 --- a/git-url/src/lib.rs +++ b/git-url/src/lib.rs @@ -38,6 +38,8 @@ pub struct Url { user: Option, /// The host to which to connect. Localhost is implied if `None`. host: Option, + /// When serializing, use the alternative forms as it was parsed as such. + serialize_alternative_form: bool, /// The port to use when connecting to a host. If `None`, standard ports depending on `scheme` will be used. pub port: Option, /// The path portion of the URL, usually the location of the git repository. @@ -61,6 +63,7 @@ impl Url { host, port, path, + serialize_alternative_form: false, } .to_bstring() .as_ref(), @@ -78,6 +81,18 @@ impl Url { } } +/// Builder +impl Url { + /// Enable alternate serialization for this url, e.g. `file:///path` becomes `/path`. + /// + /// This is automatically set correctly for parsed URLs, but can be set here for urls + /// created by constructor. + pub fn serialize_alternate_form(mut self, use_alternate_form: bool) -> Self { + self.serialize_alternative_form = use_alternate_form; + self + } +} + /// Access impl Url { /// Returns the user mentioned in the url, if present. @@ -112,8 +127,10 @@ impl Url { impl Url { /// Write this URL losslessly to `out`, ready to be parsed again. pub fn write_to(&self, mut out: impl std::io::Write) -> std::io::Result<()> { - out.write_all(self.scheme.as_str().as_bytes())?; - out.write_all(b"://")?; + if !(self.serialize_alternative_form && self.scheme == Scheme::File) { + out.write_all(self.scheme.as_str().as_bytes())?; + out.write_all(b"://")?; + } match (&self.user, &self.host) { (Some(user), Some(host)) => { out.write_all(user.as_bytes())?; @@ -148,6 +165,7 @@ impl Url { } } +/// Deserialization impl Url { /// Parse a URL from `bytes` pub fn from_bytes(bytes: &BStr) -> Result { diff --git a/git-url/src/parse.rs b/git-url/src/parse.rs index 2a2f3951cb6..e3ecbac6f2f 100644 --- a/git-url/src/parse.rs +++ b/git-url/src/parse.rs @@ -60,16 +60,13 @@ fn has_no_explicit_protocol(url: &[u8]) -> bool { url.find(b"://").is_none() } -fn possibly_strip_file_protocol(url: &[u8]) -> &[u8] { - if url.starts_with(b"file://") { - &url[b"file://".len()..] - } else { - url - } +fn try_strip_file_protocol(url: &[u8]) -> Option<&[u8]> { + url.strip_prefix(b"file://") } fn to_owned_url(url: url::Url) -> Result { Ok(crate::Url { + serialize_alternative_form: false, scheme: str_to_protocol(url.scheme())?, user: if url.username().is_empty() { None @@ -90,10 +87,12 @@ fn to_owned_url(url: url::Url) -> Result { /// For file-paths, we don't expect UTF8 encoding either. pub fn parse(input: &BStr) -> Result { let guessed_protocol = guess_protocol(input); - if possibly_strip_file_protocol(input) != input || (has_no_explicit_protocol(input) && guessed_protocol == "file") { + let path_without_protocol = try_strip_file_protocol(input); + if path_without_protocol.is_some() || (has_no_explicit_protocol(input) && guessed_protocol == "file") { return Ok(crate::Url { scheme: Scheme::File, - path: possibly_strip_file_protocol(input).into(), + path: path_without_protocol.unwrap_or(input).into(), + serialize_alternative_form: !input.starts_with(b"file://"), ..Default::default() }); } @@ -101,7 +100,7 @@ pub fn parse(input: &BStr) -> Result { let url_str = std::str::from_utf8(input)?; let mut url = match url::Url::parse(url_str) { Ok(url) => url, - Err(::url::ParseError::RelativeUrlWithoutBase) => { + Err(url::ParseError::RelativeUrlWithoutBase) => { // happens with bare paths as well as scp like paths. The latter contain a ':' past the host portion, // which we are trying to detect. url::Url::parse(&format!( diff --git a/git-url/tests/parse/file.rs b/git-url/tests/parse/file.rs index f697e4a0712..00a8e9293ab 100644 --- a/git-url/tests/parse/file.rs +++ b/git-url/tests/parse/file.rs @@ -1,7 +1,7 @@ use bstr::ByteSlice; use git_url::Scheme; -use crate::parse::{assert_url_and, assert_url_roundtrip, url}; +use crate::parse::{assert_url_and, assert_url_roundtrip, url, url_alternate}; #[test] fn file_path_with_protocol() -> crate::Result { @@ -13,15 +13,23 @@ fn file_path_with_protocol() -> crate::Result { #[test] fn file_path_without_protocol() -> crate::Result { - let url = assert_url_and("/path/to/git", url(Scheme::File, None, None, None, b"/path/to/git"))?.to_bstring(); - assert_eq!(url, "file:///path/to/git"); + let url = assert_url_and( + "/path/to/git", + url_alternate(Scheme::File, None, None, None, b"/path/to/git"), + )? + .to_bstring(); + assert_eq!(url, "/path/to/git"); Ok(()) } #[test] fn no_username_expansion_for_file_paths_without_protocol() -> crate::Result { - let url = assert_url_and("~/path/to/git", url(Scheme::File, None, None, None, b"~/path/to/git"))?.to_bstring(); - assert_eq!(url, "file://~/path/to/git"); + let url = assert_url_and( + "~/path/to/git", + url_alternate(Scheme::File, None, None, None, b"~/path/to/git"), + )? + .to_bstring(); + assert_eq!(url, "~/path/to/git"); Ok(()) } #[test] @@ -35,14 +43,17 @@ fn no_username_expansion_for_file_paths_with_protocol() -> crate::Result { #[test] fn non_utf8_file_path_without_protocol() -> crate::Result { let parsed = git_url::parse(b"/path/to\xff/git".as_bstr())?; - assert_eq!(parsed, url(Scheme::File, None, None, None, b"/path/to\xff/git",)); + assert_eq!( + parsed, + url_alternate(Scheme::File, None, None, None, b"/path/to\xff/git",) + ); let url_lossless = parsed.to_bstring(); assert_eq!( url_lossless.to_string(), - "file:///path/to�/git", + "/path/to�/git", "non-unicode is made unicode safe after conversion" ); - assert_eq!(url_lossless, &b"file:///path/to\xff/git"[..], "otherwise it's lossless"); + assert_eq!(url_lossless, &b"/path/to\xff/git"[..], "otherwise it's lossless"); Ok(()) } @@ -50,12 +61,16 @@ fn non_utf8_file_path_without_protocol() -> crate::Result { fn relative_file_path_without_protocol() -> crate::Result { let parsed = assert_url_and( "../../path/to/git", - url(Scheme::File, None, None, None, b"../../path/to/git"), + url_alternate(Scheme::File, None, None, None, b"../../path/to/git"), + )? + .to_bstring(); + assert_eq!(parsed, "../../path/to/git"); + let url = assert_url_and( + "path/to/git", + url_alternate(Scheme::File, None, None, None, b"path/to/git"), )? .to_bstring(); - assert_eq!(parsed, "file://../../path/to/git"); - let url = assert_url_and("path/to/git", url(Scheme::File, None, None, None, b"path/to/git"))?.to_bstring(); - assert_eq!(url, "file://path/to/git"); + assert_eq!(url, "path/to/git"); Ok(()) } @@ -63,23 +78,26 @@ fn relative_file_path_without_protocol() -> crate::Result { fn interior_relative_file_path_without_protocol() -> crate::Result { let url = assert_url_and( "/abs/path/../../path/to/git", - url(Scheme::File, None, None, None, b"/abs/path/../../path/to/git"), + url_alternate(Scheme::File, None, None, None, b"/abs/path/../../path/to/git"), )? .to_bstring(); - assert_eq!(url, "file:///abs/path/../../path/to/git"); + assert_eq!(url, "/abs/path/../../path/to/git"); Ok(()) } mod windows { use git_url::Scheme; - use crate::parse::{assert_url_and, assert_url_roundtrip, url}; + use crate::parse::{assert_url_and, assert_url_roundtrip, url, url_alternate}; #[test] fn file_path_without_protocol() -> crate::Result { - let url = - assert_url_and("x:/path/to/git", url(Scheme::File, None, None, None, b"x:/path/to/git"))?.to_bstring(); - assert_eq!(url, "file://x:/path/to/git"); + let url = assert_url_and( + "x:/path/to/git", + url_alternate(Scheme::File, None, None, None, b"x:/path/to/git"), + )? + .to_bstring(); + assert_eq!(url, "x:/path/to/git"); Ok(()) } @@ -87,10 +105,10 @@ mod windows { fn file_path_with_backslashes_without_protocol() -> crate::Result { let url = assert_url_and( "x:\\path\\to\\git", - url(Scheme::File, None, None, None, b"x:\\path\\to\\git"), + url_alternate(Scheme::File, None, None, None, b"x:\\path\\to\\git"), )? .to_bstring(); - assert_eq!(url, "file://x:\\path\\to\\git"); + assert_eq!(url, "x:\\path\\to\\git"); Ok(()) } diff --git a/git-url/tests/parse/mod.rs b/git-url/tests/parse/mod.rs index 47d8a6e1798..64c05254bbf 100644 --- a/git-url/tests/parse/mod.rs +++ b/git-url/tests/parse/mod.rs @@ -43,6 +43,16 @@ fn url( .expect("valid") } +fn url_alternate( + protocol: Scheme, + user: impl Into>, + host: impl Into>, + port: impl Into>, + path: &'static [u8], +) -> git_url::Url { + url(protocol, user, host, port, path).serialize_alternate_form(true) +} + mod file; mod invalid; mod ssh; From 39ce98ba9a427b8cea1b843f333c2e7de300499c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 10:42:30 +0800 Subject: [PATCH 17/42] feat: (mostly) lossless roundtripping of scp-like urls. Previously `git@host:path` would turn into `ssh://git@host/path`, which now remains exactly as is. --- git-url/src/lib.rs | 9 +++++++-- git-url/src/parse.rs | 20 ++++++++++++-------- git-url/tests/parse/ssh.rs | 18 +++++++++--------- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/git-url/src/lib.rs b/git-url/src/lib.rs index 6da0d872fb3..ed3deedd2c3 100644 --- a/git-url/src/lib.rs +++ b/git-url/src/lib.rs @@ -127,7 +127,7 @@ impl Url { impl Url { /// Write this URL losslessly to `out`, ready to be parsed again. pub fn write_to(&self, mut out: impl std::io::Write) -> std::io::Result<()> { - if !(self.serialize_alternative_form && self.scheme == Scheme::File) { + if !(self.serialize_alternative_form && (self.scheme == Scheme::File || self.scheme == Scheme::Ssh)) { out.write_all(self.scheme.as_str().as_bytes())?; out.write_all(b"://")?; } @@ -146,7 +146,12 @@ impl Url { if let Some(port) = &self.port { write!(&mut out, ":{}", port)?; } - out.write_all(&self.path)?; + if self.serialize_alternative_form && self.scheme == Scheme::Ssh { + out.write_all(b":")?; + out.write_all(&self.path[1..])?; + } else { + out.write_all(&self.path)?; + } Ok(()) } diff --git a/git-url/src/parse.rs b/git-url/src/parse.rs index e3ecbac6f2f..c6f33f6c4e0 100644 --- a/git-url/src/parse.rs +++ b/git-url/src/parse.rs @@ -98,16 +98,19 @@ pub fn parse(input: &BStr) -> Result { } let url_str = std::str::from_utf8(input)?; - let mut url = match url::Url::parse(url_str) { - Ok(url) => url, + let (mut url, mut sanitized_scp) = match url::Url::parse(url_str) { + Ok(url) => (url, false), Err(url::ParseError::RelativeUrlWithoutBase) => { // happens with bare paths as well as scp like paths. The latter contain a ':' past the host portion, // which we are trying to detect. - url::Url::parse(&format!( - "{}://{}", - guessed_protocol, - sanitize_for_protocol(guessed_protocol, url_str) - ))? + ( + url::Url::parse(&format!( + "{}://{}", + guessed_protocol, + sanitize_for_protocol(guessed_protocol, url_str) + ))?, + true, + ) } Err(err) => return Err(err.into()), }; @@ -115,6 +118,7 @@ pub fn parse(input: &BStr) -> Result { if url.scheme().find('.').is_some() { // try again with prefixed protocol url = url::Url::parse(&format!("ssh://{}", sanitize_for_protocol("ssh", url_str)))?; + sanitized_scp = true; } if url.scheme() != "rad" && url.path().is_empty() { return Err(Error::EmptyPath); @@ -123,5 +127,5 @@ pub fn parse(input: &BStr) -> Result { return Err(Error::RelativeUrl { url: url.into() }); } - to_owned_url(url) + to_owned_url(url).map(|url| url.serialize_alternate_form(sanitized_scp)) } diff --git a/git-url/tests/parse/ssh.rs b/git-url/tests/parse/ssh.rs index 349069f398a..a5181211f61 100644 --- a/git-url/tests/parse/ssh.rs +++ b/git-url/tests/parse/ssh.rs @@ -1,6 +1,6 @@ use git_url::Scheme; -use crate::parse::{assert_url_and, assert_url_roundtrip, url}; +use crate::parse::{assert_url_and, assert_url_roundtrip, url, url_alternate}; #[test] fn without_user_and_without_port() -> crate::Result { @@ -51,10 +51,10 @@ fn with_user_and_without_port() -> crate::Result { fn scp_like_without_user() -> crate::Result { let url = assert_url_and( "host.xz:path/to/git", - url(Scheme::Ssh, None, "host.xz", None, b"/path/to/git"), + url_alternate(Scheme::Ssh, None, "host.xz", None, b"/path/to/git"), )? .to_bstring(); - assert_eq!(url, "ssh://host.xz/path/to/git"); + assert_eq!(url, "host.xz:path/to/git"); Ok(()) } @@ -62,10 +62,10 @@ fn scp_like_without_user() -> crate::Result { fn scp_like_without_user_and_username_expansion_without_username() -> crate::Result { let url = assert_url_and( "host.xz:~/to/git", - url(Scheme::Ssh, None, "host.xz", None, b"/~/to/git"), + url_alternate(Scheme::Ssh, None, "host.xz", None, b"/~/to/git"), )? .to_bstring(); - assert_eq!(url, "ssh://host.xz/~/to/git"); + assert_eq!(url, "host.xz:~/to/git"); Ok(()) } @@ -73,10 +73,10 @@ fn scp_like_without_user_and_username_expansion_without_username() -> crate::Res fn scp_like_without_user_and_username_expansion_with_username() -> crate::Result { let url = assert_url_and( "host.xz:~byron/to/git", - url(Scheme::Ssh, None, "host.xz", None, b"/~byron/to/git"), + url_alternate(Scheme::Ssh, None, "host.xz", None, b"/~byron/to/git"), )? .to_bstring(); - assert_eq!(url, "ssh://host.xz/~byron/to/git"); + assert_eq!(url, "host.xz:~byron/to/git"); Ok(()) } @@ -84,9 +84,9 @@ fn scp_like_without_user_and_username_expansion_with_username() -> crate::Result fn scp_like_with_user_and_relative_path_turns_into_absolute_path() -> crate::Result { let url = assert_url_and( "user@host.xz:./relative", - url(Scheme::Ssh, "user", "host.xz", None, b"/relative"), + url_alternate(Scheme::Ssh, "user", "host.xz", None, b"/relative"), )? .to_bstring(); - assert_eq!(url, "ssh://user@host.xz/relative"); + assert_eq!(url, "user@host.xz:relative"); Ok(()) } From 5926322c7dc9ef45c0f8c7dc50551d0bf1800ada Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 10:45:31 +0800 Subject: [PATCH 18/42] more assurance we understand how relative paths in scp-like urls work --- git-url/tests/parse/ssh.rs | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/git-url/tests/parse/ssh.rs b/git-url/tests/parse/ssh.rs index a5181211f61..6654949aea9 100644 --- a/git-url/tests/parse/ssh.rs +++ b/git-url/tests/parse/ssh.rs @@ -88,5 +88,12 @@ fn scp_like_with_user_and_relative_path_turns_into_absolute_path() -> crate::Res )? .to_bstring(); assert_eq!(url, "user@host.xz:relative"); + + let url = assert_url_and( + "user@host.xz:../relative", + url_alternate(Scheme::Ssh, "user", "host.xz", None, b"/../relative"), + )? + .to_bstring(); + assert_eq!(url, "user@host.xz:relative"); Ok(()) } From a1068a3f24ce55b9eb92b97c84f650f901c7a5d3 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 10:46:17 +0800 Subject: [PATCH 19/42] adjust to changes in `git-url` --- git-repository/tests/remote/save.rs | 23 +++++++++++------------ git-repository/tests/repository/remote.rs | 2 +- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 64381ed36e2..c27c54f3164 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -20,13 +20,12 @@ mod save_to { "…by checking only the parts that are similar" ); - // TODO: fix this - // let previous_remote_state = repo - // .config_snapshot() - // .plumbing() - // .section("remote", Some("origin")) - // .expect("present") - // .to_bstring(); + let previous_remote_state = repo + .config_snapshot() + .plumbing() + .section("remote", Some("origin")) + .expect("present") + .to_bstring(); let mut config = repo.config_snapshot().plumbing().clone(); remote.save_to(&mut config)?; assert_eq!( @@ -34,11 +33,11 @@ mod save_to { 2, "amount of remotes are unaltered" ); - // assert_eq!( - // config.section("remote", Some("origin")).expect("present").to_bstring(), - // previous_remote_state, - // "the serialization doesn't modify anything" - // ); + assert_eq!( + config.section("remote", Some("origin")).expect("present").to_bstring(), + previous_remote_state, + "the serialization doesn't modify anything" + ); Ok(()) } } diff --git a/git-repository/tests/repository/remote.rs b/git-repository/tests/repository/remote.rs index 1da830e4b85..4028a0b8e9b 100644 --- a/git-repository/tests/repository/remote.rs +++ b/git-repository/tests/repository/remote.rs @@ -16,7 +16,7 @@ mod remote_at { let mut remote = remote.push_url("user@host.xz:./relative")?; assert_eq!( remote.url(Direction::Push).unwrap().to_bstring(), - "ssh://user@host.xz/relative" + "user@host.xz:relative" ); assert_eq!(remote.url(Direction::Fetch).unwrap().to_bstring(), fetch_url); From 22d3b37ea6239170a478b859361a7d1d7ba01a9a Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 16:15:23 +0800 Subject: [PATCH 20/42] feat: `Url::try_from(path: &std::path::Path)` for more convenient instantiation. --- git-repository/src/clone.rs | 0 git-repository/tests/clone/mod.rs | 0 git-url/src/impls.rs | 14 +++++++++++++- 3 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 git-repository/src/clone.rs create mode 100644 git-repository/tests/clone/mod.rs diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs new file mode 100644 index 00000000000..e69de29bb2d diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs new file mode 100644 index 00000000000..e69de29bb2d diff --git a/git-url/src/impls.rs b/git-url/src/impls.rs index 1288cd2e2f1..e191f4c79a9 100644 --- a/git-url/src/impls.rs +++ b/git-url/src/impls.rs @@ -1,4 +1,7 @@ -use std::{convert::TryFrom, path::PathBuf}; +use std::{ + convert::TryFrom, + path::{Path, PathBuf}, +}; use bstr::BStr; @@ -42,6 +45,15 @@ impl TryFrom for Url { } } +impl TryFrom<&Path> for Url { + type Error = parse::Error; + + fn try_from(value: &Path) -> Result { + use std::convert::TryInto; + git_path::into_bstr(value).try_into() + } +} + impl TryFrom<&std::ffi::OsStr> for Url { type Error = parse::Error; From ef1e7834875fd2e1ab793a17a393ad3e5470059c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 16:22:57 +0800 Subject: [PATCH 21/42] Api sketch to show how clones and bare clones can be done. It's explicitly a multi-step process and maybe the function names could represent that better. --- git-repository/src/clone.rs | 66 ++++++++++++++++++++++++++ git-repository/src/lib.rs | 49 +++++++++++++++++++ git-repository/tests/clone/mod.rs | 36 ++++++++++++++ git-repository/tests/git-with-regex.rs | 1 + git-repository/tests/git.rs | 2 + 5 files changed, 154 insertions(+) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index e69de29bb2d..2ff511b6410 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -0,0 +1,66 @@ +use crate::Repository; +use std::convert::TryInto; + +/// A utility to collect configuration on how to fetch from a remote and possibly create a working tree locally. +pub struct Prepare { + /// A freshly initialized repository which is owned by us, or `None` if it was handed to the user + repo: Option, + /// The url to clone from + #[allow(dead_code)] + url: git_url::Url, +} + +impl Drop for Prepare { + fn drop(&mut self) { + if let Some(repo) = self.repo.take() { + std::fs::remove_dir_all(repo.work_dir().unwrap_or_else(|| repo.path())).ok(); + } + } +} + +impl Into for Prepare { + fn into(self) -> Repository { + self.persist() + } +} + +/// The error returned by [`Prepare::new()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error(transparent)] + Init(#[from] crate::init::Error), + #[error(transparent)] + UrlParse(#[from] git_url::parse::Error), +} + +/// Instantiation +impl Prepare { + /// Create a new repository at `path` with `crate_opts` which is ready to clone from `url`, possibly after making additional adjustments to + /// configuration and settings. + /// + /// Note that this is merely a handle to perform the actual connection to the remote, and if any of it fails the freshly initialized repository + /// will be removed automatically as soon as this instance drops. + pub fn new( + url: Url, + path: impl AsRef, + create_opts: crate::create::Options, + open_opts: crate::open::Options, + ) -> Result + where + Url: TryInto, + git_url::parse::Error: From, + { + let url = url.try_into().map_err(git_url::parse::Error::from)?; + let repo = crate::ThreadSafeRepository::init_opts(path, create_opts, open_opts)?.to_thread_local(); + Ok(Prepare { url, repo: Some(repo) }) + } +} + +/// Access +impl Prepare { + /// Persist the contained repository as is even if an error may have occurred when interacting with the remote or checking out the main working tree. + pub fn persist(mut self) -> Repository { + self.repo.take().expect("present and consumed once") + } +} diff --git a/git-repository/src/lib.rs b/git-repository/src/lib.rs index 8c957e1fdfc..0ca49c80554 100644 --- a/git-repository/src/lib.rs +++ b/git-repository/src/lib.rs @@ -183,6 +183,8 @@ pub use types::{ Worktree, }; +/// +pub mod clone; pub mod commit; pub mod head; pub mod id; @@ -220,6 +222,53 @@ pub fn init_bare(directory: impl AsRef) -> Result(url: Url, path: impl AsRef) -> Result +where + Url: std::convert::TryInto, + git_url::parse::Error: From, +{ + clone::Prepare::new( + url, + path, + create::Options { + bare: true, + fs_capabilities: None, + }, + open_opts_with_git_binary_config(), + ) +} + +/// Create a platform for configuring a clone with main working tree from `url` to the local `path`, using default options for opening it +/// (but amended with using configuration from the git installation to ensure all authentication options are honored). +/// +/// See [`clone::Prepare::new()] for a function to take full control over all options. +pub fn clone(url: Url, path: impl AsRef) -> Result +where + Url: std::convert::TryInto, + git_url::parse::Error: From, +{ + clone::Prepare::new( + url, + path, + create::Options { + bare: false, + fs_capabilities: None, + }, + open_opts_with_git_binary_config(), + ) +} + +fn open_opts_with_git_binary_config() -> open::Options { + use git_sec::trust::DefaultForLevel; + let mut opts = open::Options::default_for_level(git_sec::Trust::Full); + opts.permissions.config.git_binary = true; + opts +} + /// See [ThreadSafeRepository::open()], but returns a [`Repository`] instead. pub fn open(directory: impl Into) -> Result { ThreadSafeRepository::open(directory).map(Into::into) diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index e69de29bb2d..8b76eea464a 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -0,0 +1,36 @@ +use crate::remote; +use git_repository as git; + +#[test] +fn persist() -> crate::Result { + let tmp = git_testtools::tempfile::TempDir::new()?; + let repo = git::clone_bare(remote::repo("base").path(), tmp.path())?.persist(); + assert_eq!(repo.is_bare(), true, "repo is now ours and remains"); + Ok(()) +} + +#[test] +fn clone_bare_into_empty_directory() -> crate::Result { + let tmp = git_testtools::tempfile::TempDir::new()?; + // this breaks isolation, but shouldn't be affecting the test. If so, use isolation options for opening the repo. + let prep = git::clone_bare(remote::repo("base").path(), tmp.path())?; + let head = tmp.path().join("HEAD"); + assert!(head.is_file(), "now a bare basic repo is present"); + drop(prep); + + assert!(!head.is_file(), "we cleanup if the clone isn't followed through"); + Ok(()) +} + +#[test] +fn clone_into_empty_directory() -> crate::Result { + let tmp = git_testtools::tempfile::TempDir::new()?; + // this breaks isolation, but shouldn't be affecting the test. If so, use isolation options for opening the repo. + let prep = git::clone(remote::repo("base").path(), tmp.path())?; + let head = tmp.path().join(".git").join("HEAD"); + assert!(head.is_file(), "now a basic repo is present"); + drop(prep); + + assert!(!head.is_file(), "we cleanup if the clone isn't followed through"); + Ok(()) +} diff --git a/git-repository/tests/git-with-regex.rs b/git-repository/tests/git-with-regex.rs index 0be6ff1efe4..dbaf2fb20e3 100644 --- a/git-repository/tests/git-with-regex.rs +++ b/git-repository/tests/git-with-regex.rs @@ -1,6 +1,7 @@ mod util; use util::*; +mod clone; mod commit; mod head; mod id; diff --git a/git-repository/tests/git.rs b/git-repository/tests/git.rs index 305de9db9dd..661db12d9ef 100644 --- a/git-repository/tests/git.rs +++ b/git-repository/tests/git.rs @@ -3,6 +3,8 @@ mod util; #[cfg(not(feature = "regex"))] use util::*; +#[cfg(not(feature = "regex"))] +mod clone; #[cfg(not(feature = "regex"))] mod commit; #[cfg(not(feature = "regex"))] From d7f495d6d5c436decc6ea6236f2129a4758facc0 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 16:30:58 +0800 Subject: [PATCH 22/42] refactor --- git-repository/src/clone.rs | 102 ++++++++++++++++-------------- git-repository/src/lib.rs | 10 ++- git-repository/tests/clone/mod.rs | 6 +- 3 files changed, 64 insertions(+), 54 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 2ff511b6410..9fe567fd6d5 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -1,66 +1,70 @@ -use crate::Repository; -use std::convert::TryInto; - /// A utility to collect configuration on how to fetch from a remote and possibly create a working tree locally. pub struct Prepare { /// A freshly initialized repository which is owned by us, or `None` if it was handed to the user - repo: Option, + repo: Option, /// The url to clone from #[allow(dead_code)] url: git_url::Url, } -impl Drop for Prepare { - fn drop(&mut self) { - if let Some(repo) = self.repo.take() { - std::fs::remove_dir_all(repo.work_dir().unwrap_or_else(|| repo.path())).ok(); - } +/// +pub mod prepare { + use crate::clone::Prepare; + use crate::Repository; + use std::convert::TryInto; + + /// The error returned by [`Prepare::new()`]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error(transparent)] + Init(#[from] crate::init::Error), + #[error(transparent)] + UrlParse(#[from] git_url::parse::Error), } -} -impl Into for Prepare { - fn into(self) -> Repository { - self.persist() + /// Instantiation + impl Prepare { + /// Create a new repository at `path` with `crate_opts` which is ready to clone from `url`, possibly after making additional adjustments to + /// configuration and settings. + /// + /// Note that this is merely a handle to perform the actual connection to the remote, and if any of it fails the freshly initialized repository + /// will be removed automatically as soon as this instance drops. + pub fn new( + url: Url, + path: impl AsRef, + create_opts: crate::create::Options, + open_opts: crate::open::Options, + ) -> Result + where + Url: TryInto, + git_url::parse::Error: From, + { + let url = url.try_into().map_err(git_url::parse::Error::from)?; + let repo = crate::ThreadSafeRepository::init_opts(path, create_opts, open_opts)?.to_thread_local(); + Ok(Prepare { url, repo: Some(repo) }) + } } -} -/// The error returned by [`Prepare::new()`]. -#[derive(Debug, thiserror::Error)] -#[allow(missing_docs)] -pub enum Error { - #[error(transparent)] - Init(#[from] crate::init::Error), - #[error(transparent)] - UrlParse(#[from] git_url::parse::Error), -} + /// Access + impl Prepare { + /// Persist the contained repository as is even if an error may have occurred when interacting with the remote or checking out the main working tree. + pub fn persist(mut self) -> Repository { + self.repo.take().expect("present and consumed once") + } + } -/// Instantiation -impl Prepare { - /// Create a new repository at `path` with `crate_opts` which is ready to clone from `url`, possibly after making additional adjustments to - /// configuration and settings. - /// - /// Note that this is merely a handle to perform the actual connection to the remote, and if any of it fails the freshly initialized repository - /// will be removed automatically as soon as this instance drops. - pub fn new( - url: Url, - path: impl AsRef, - create_opts: crate::create::Options, - open_opts: crate::open::Options, - ) -> Result - where - Url: TryInto, - git_url::parse::Error: From, - { - let url = url.try_into().map_err(git_url::parse::Error::from)?; - let repo = crate::ThreadSafeRepository::init_opts(path, create_opts, open_opts)?.to_thread_local(); - Ok(Prepare { url, repo: Some(repo) }) + impl Drop for Prepare { + fn drop(&mut self) { + if let Some(repo) = self.repo.take() { + std::fs::remove_dir_all(repo.work_dir().unwrap_or_else(|| repo.path())).ok(); + } + } } -} -/// Access -impl Prepare { - /// Persist the contained repository as is even if an error may have occurred when interacting with the remote or checking out the main working tree. - pub fn persist(mut self) -> Repository { - self.repo.take().expect("present and consumed once") + impl Into for Prepare { + fn into(self) -> Repository { + self.persist() + } } } diff --git a/git-repository/src/lib.rs b/git-repository/src/lib.rs index 0ca49c80554..1ac52181064 100644 --- a/git-repository/src/lib.rs +++ b/git-repository/src/lib.rs @@ -226,7 +226,10 @@ pub fn init_bare(directory: impl AsRef) -> Result(url: Url, path: impl AsRef) -> Result +pub fn prepare_clone_bare( + url: Url, + path: impl AsRef, +) -> Result where Url: std::convert::TryInto, git_url::parse::Error: From, @@ -246,7 +249,10 @@ where /// (but amended with using configuration from the git installation to ensure all authentication options are honored). /// /// See [`clone::Prepare::new()] for a function to take full control over all options. -pub fn clone(url: Url, path: impl AsRef) -> Result +pub fn prepare_clone( + url: Url, + path: impl AsRef, +) -> Result where Url: std::convert::TryInto, git_url::parse::Error: From, diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 8b76eea464a..becfa107206 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -4,7 +4,7 @@ use git_repository as git; #[test] fn persist() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; - let repo = git::clone_bare(remote::repo("base").path(), tmp.path())?.persist(); + let repo = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())?.persist(); assert_eq!(repo.is_bare(), true, "repo is now ours and remains"); Ok(()) } @@ -13,7 +13,7 @@ fn persist() -> crate::Result { fn clone_bare_into_empty_directory() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; // this breaks isolation, but shouldn't be affecting the test. If so, use isolation options for opening the repo. - let prep = git::clone_bare(remote::repo("base").path(), tmp.path())?; + let prep = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())?; let head = tmp.path().join("HEAD"); assert!(head.is_file(), "now a bare basic repo is present"); drop(prep); @@ -26,7 +26,7 @@ fn clone_bare_into_empty_directory() -> crate::Result { fn clone_into_empty_directory() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; // this breaks isolation, but shouldn't be affecting the test. If so, use isolation options for opening the repo. - let prep = git::clone(remote::repo("base").path(), tmp.path())?; + let prep = git::prepare_clone(remote::repo("base").path(), tmp.path())?; let head = tmp.path().join(".git").join("HEAD"); assert!(head.is_file(), "now a basic repo is present"); drop(prep); From 615a3a90ffb67c76e00bf3a024aaafa67ef148cb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 16:43:23 +0800 Subject: [PATCH 23/42] sketch a way to configure remotes --- git-repository/src/clone.rs | 25 ++++++++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 9fe567fd6d5..c31282823bc 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -2,6 +2,9 @@ pub struct Prepare { /// A freshly initialized repository which is owned by us, or `None` if it was handed to the user repo: Option, + /// A function to configure a remote prior to fetching a pack. + configure_remote: + Option) -> Result, crate::remote::init::Error>>>, /// The url to clone from #[allow(dead_code)] url: git_url::Url, @@ -42,7 +45,27 @@ pub mod prepare { { let url = url.try_into().map_err(git_url::parse::Error::from)?; let repo = crate::ThreadSafeRepository::init_opts(path, create_opts, open_opts)?.to_thread_local(); - Ok(Prepare { url, repo: Some(repo) }) + Ok(Prepare { + url, + repo: Some(repo), + configure_remote: None, + }) + } + } + + /// Builder + impl Prepare { + /// Use `f` to apply arbitrary changes to the remote that is about to be used to fetch a pack. + /// + /// The passed in `remote` will be anonymous and pre-configured to be a default remote as we know it from git-clone. + /// It is not yet present in the configuration of the repository, + /// but each change it will eventually be written to the configuration prior to performing a the fetch operation. + pub fn configure_remote( + mut self, + f: impl FnOnce(crate::Remote<'_>) -> Result, crate::remote::init::Error> + 'static, + ) -> Self { + self.configure_remote = Some(Box::new(f)); + self } } From 6cbea960b798181f46e736d509eed5a44b91e0c8 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 16:56:46 +0800 Subject: [PATCH 24/42] =?UTF-8?q?start=20adding=20support=20for=20naming?= =?UTF-8?q?=20the=20remote,=20but=E2=80=A6?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …realize that subsection/remote-name validation is incorrect here and in another place that needs fixing. --- git-repository/src/clone.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index c31282823bc..2271933f391 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -2,6 +2,9 @@ pub struct Prepare { /// 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. + #[allow(dead_code)] + remote_name: Option, /// A function to configure a remote prior to fetching a pack. configure_remote: Option) -> Result, crate::remote::init::Error>>>, @@ -48,6 +51,7 @@ pub mod prepare { Ok(Prepare { url, repo: Some(repo), + remote_name: None, configure_remote: None, }) } @@ -57,7 +61,7 @@ pub mod prepare { impl Prepare { /// Use `f` to apply arbitrary changes to the remote that is about to be used to fetch a pack. /// - /// The passed in `remote` will be anonymous and pre-configured to be a default remote as we know it from git-clone. + /// The passed in `remote` will be un-named and pre-configured to be a default remote as we know it from git-clone. /// It is not yet present in the configuration of the repository, /// but each change it will eventually be written to the configuration prior to performing a the fetch operation. pub fn configure_remote( From 8c8fba2f5d6b464bb9fc275bbf2db89635e75d43 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 16:59:27 +0800 Subject: [PATCH 25/42] thanks clippy --- git-repository/src/clone.rs | 11 ++++++----- git-repository/tests/clone/mod.rs | 2 +- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 2271933f391..bdcfcc259d1 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -1,3 +1,5 @@ +type ConfigureRemoteFn = Box) -> Result, crate::remote::init::Error>>; + /// A utility to collect configuration on how to fetch from a remote and possibly create a working tree locally. pub struct Prepare { /// A freshly initialized repository which is owned by us, or `None` if it was handed to the user @@ -6,8 +8,7 @@ pub struct Prepare { #[allow(dead_code)] remote_name: Option, /// A function to configure a remote prior to fetching a pack. - configure_remote: - Option) -> Result, crate::remote::init::Error>>>, + configure_remote: Option, /// The url to clone from #[allow(dead_code)] url: git_url::Url, @@ -89,9 +90,9 @@ pub mod prepare { } } - impl Into for Prepare { - fn into(self) -> Repository { - self.persist() + impl From for Repository { + fn from(prep: Prepare) -> Self { + prep.persist() } } } diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index becfa107206..152f0449717 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -5,7 +5,7 @@ use git_repository as git; fn persist() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; let repo = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())?.persist(); - assert_eq!(repo.is_bare(), true, "repo is now ours and remains"); + assert!(repo.is_bare(), "repo is now ours and remains"); Ok(()) } From aa5d66f60bd6c9ef404ebc120b613e0cf055b2c9 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 20:46:17 +0800 Subject: [PATCH 26/42] feat: add `parse::section::header::is_valid_subsection()` function. It can be useful to validate subsection names without having to construct an entire `Header` (which also includes a name). --- git-config/src/parse/section/header.rs | 26 ++++++++++++++++++++++++-- git-config/src/parse/section/mod.rs | 4 ++-- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/git-config/src/parse/section/header.rs b/git-config/src/parse/section/header.rs index fdedb27e00d..c4c13a97365 100644 --- a/git-config/src/parse/section/header.rs +++ b/git-config/src/parse/section/header.rs @@ -41,9 +41,13 @@ impl<'a> Header<'a> { } } +/// Return true if `name` is valid as subsection name, like `origin` in `[remote "origin"]`. +pub fn is_valid_subsection(name: &BStr) -> bool { + name.find_byteset(b"\n\0").is_none() +} + fn validated_subsection(name: Cow<'_, BStr>) -> Result, Error> { - name.find_byteset(b"\n\0") - .is_none() + is_valid_subsection(name.as_ref()) .then(|| name) .ok_or(Error::InvalidSubSection) } @@ -55,6 +59,24 @@ fn validated_name(name: Cow<'_, BStr>) -> Result, Error> { .ok_or(Error::InvalidName) } +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn empty_header_names_are_legal() { + assert!(Header::new("", None).is_ok(), "yes, git allows this, so do we"); + } + + #[test] + fn empty_header_sub_names_are_legal() { + assert!( + Header::new("remote", Some("".into())).is_ok(), + "yes, git allows this, so do we" + ); + } +} + impl Header<'_> { ///Return true if this is a header like `[legacy.subsection]`, or false otherwise. pub fn is_legacy(&self) -> bool { diff --git a/git-config/src/parse/section/mod.rs b/git-config/src/parse/section/mod.rs index 459ad5906ef..040efa944b0 100644 --- a/git-config/src/parse/section/mod.rs +++ b/git-config/src/parse/section/mod.rs @@ -160,10 +160,10 @@ mod types { generate_case_insensitive!( Name, name, - "Valid names consist alphanumeric characters or dashes.", + "Valid names consist of alphanumeric characters or dashes.", is_valid_name, bstr::BStr, - "Wrapper struct for section header names, like `includeIf`, since these are case-insensitive." + "Wrapper struct for section header names, like `remote`, since these are case-insensitive." ); generate_case_insensitive!( From b2c9af1cf7eedfb618c47d0598cfcef636e793ff Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 21:16:13 +0800 Subject: [PATCH 27/42] Another test to validate components must not be empty --- git-refspec/tests/parse/invalid.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/git-refspec/tests/parse/invalid.rs b/git-refspec/tests/parse/invalid.rs index 436d4a21f1c..e3cb5b1a46a 100644 --- a/git-refspec/tests/parse/invalid.rs +++ b/git-refspec/tests/parse/invalid.rs @@ -7,6 +7,14 @@ fn empty() { assert!(matches!(try_parse("", Operation::Push).unwrap_err(), Error::Empty)); } +#[test] +fn empty_component() { + assert!(matches!( + try_parse("refs/heads/test:refs/remotes//test", Operation::Fetch).unwrap_err(), + Error::ReferenceName(git_validate::refname::Error::RepeatedSlash) + )); +} + #[test] fn complex_patterns_with_more_than_one_asterisk() { for op in [Operation::Fetch, Operation::Push] { From 1fb97fbfe893d6a8030e3ef0bae34f40a3e9b7e6 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 21:16:33 +0800 Subject: [PATCH 28/42] properly validate remotes when instantiating them and when naming them --- git-repository/src/remote/init.rs | 4 +++- git-repository/src/remote/mod.rs | 24 ++++++++++++++++++++++++ git-repository/src/remote/save.rs | 21 +++++++++++++++++---- git-repository/tests/remote/mod.rs | 7 +++++++ git-repository/tests/remote/save.rs | 6 +++--- 5 files changed, 54 insertions(+), 8 deletions(-) diff --git a/git-repository/src/remote/init.rs b/git-repository/src/remote/init.rs index 85dbf43d89d..a43aaaff69f 100644 --- a/git-repository/src/remote/init.rs +++ b/git-repository/src/remote/init.rs @@ -11,6 +11,8 @@ 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")] @@ -42,7 +44,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: name.map(|name| remote::name::validated(name)).transpose()?, url, url_alias, push_url, diff --git a/git-repository/src/remote/mod.rs b/git-repository/src/remote/mod.rs index fadd402fef0..1330db8d043 100644 --- a/git-repository/src/remote/mod.rs +++ b/git-repository/src/remote/mod.rs @@ -22,6 +22,30 @@ mod build; mod errors; pub use errors::find; +/// +pub mod name { + /// The error returned by [validate()]. + #[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/remote/save.rs b/git-repository/src/remote/save.rs index 0d7838596d2..530992bcc3a 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -9,6 +9,18 @@ pub enum Error { NameMissing { url: git_url::Url }, } +/// The error returned by [`Remote::save_as_to()`]. +/// +/// Note that this type should rather be in the `as` module, but cannot be as it's part of the Rust syntax. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum AsError { + #[error(transparent)] + Save(#[from] Error), + #[error(transparent)] + Name(#[from] crate::remote::name::Error), +} + /// Serialize into git-config. impl Remote<'_> { /// Save ourselves to the given `config` if we are a named remote or fail otherwise. @@ -78,14 +90,15 @@ impl Remote<'_> { /// and the caller should account for that. pub fn save_as_to( &mut self, - name: git_config::parse::section::Name<'_>, + name: impl Into, config: &mut git_config::File<'static>, - ) -> Result<(), Error> { + ) -> Result<(), AsError> { + let name = crate::remote::name::validated(name)?; let prev_name = self.name.take(); - self.name = Some(name.to_string()); + self.name = name.into(); self.save_to(config).map_err(|err| { self.name = prev_name; - err + err.into() }) } } diff --git a/git-repository/tests/remote/mod.rs b/git-repository/tests/remote/mod.rs index b634044ef5b..4ce5f8638a5 100644 --- a/git-repository/tests/remote/mod.rs +++ b/git-repository/tests/remote/mod.rs @@ -20,3 +20,10 @@ mod connect; mod fetch; mod ref_map; mod save; +mod name { + use git_repository as git; + #[test] + fn empty_is_invalid() { + assert!(git::remote::name::validated("").is_err()); + } +} diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index c27c54f3164..1025647ffbc 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -79,11 +79,11 @@ mod save_as_to { ); assert_eq!(remote.name(), None); let mut config = git::config::File::default(); - remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; + remote.save_as_to(remote_name, &mut config)?; let expected = "[remote \"origin\"]\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n"; assert_eq!(uniformize(config.to_string()), expected); - remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; + remote.save_as_to(remote_name, &mut config)?; assert_eq!( uniformize(config.to_string()), expected, @@ -103,7 +103,7 @@ mod save_as_to { .expect("works"); existing_section.push("free".try_into().unwrap(), Some("should not be removed".into())) } - remote.save_as_to(remote_name.try_into().expect("valid name"), &mut config)?; + remote.save_as_to(remote_name, &mut config)?; assert_eq!( uniformize(config.to_string()), "[remote \"origin\"]\n\tfree = should not be removed\n\turl = https://example.com/path\n\tpushurl = https://ein.hub/path\n\tfetch = +refs/heads/*:refs/remotes/any/*\n\tfetch = refs/heads/special:refs/heads/special-upstream\n\tpush = refs/heads/main:refs/heads/main\n\tpush = :\n[unrelated]\n\ta = value\n[initially-empty-not-removed \"name\"]\n", From 3f8b0e5c480efcdc73ce781bc3e9e9b156ec6551 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 21:20:40 +0800 Subject: [PATCH 29/42] make it possible to renmae the remote --- git-repository/src/clone.rs | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index bdcfcc259d1..1361dd6ea7e 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -5,7 +5,6 @@ pub struct Prepare { /// 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. - #[allow(dead_code)] remote_name: Option, /// A function to configure a remote prior to fetching a pack. configure_remote: Option, @@ -72,6 +71,15 @@ pub mod prepare { self.configure_remote = Some(Box::new(f)); self } + + /// Set the remote's name to the given value after it was configured using the function provided via + /// [`configure_remote()`][Self::configure_remote()]. + /// + /// If not set here, it defaults to `origin`. + pub fn remote_name(mut self, name: impl Into) -> Result { + self.remote_name = Some(crate::remote::name::validated(name)?); + Ok(self) + } } /// Access From f65a1f4b08bdacd92baf118722a45a02a71117f1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sun, 9 Oct 2022 21:21:42 +0800 Subject: [PATCH 30/42] thanks clippy --- git-repository/src/remote/init.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/git-repository/src/remote/init.rs b/git-repository/src/remote/init.rs index a43aaaff69f..bb29280c7a6 100644 --- a/git-repository/src/remote/init.rs +++ b/git-repository/src/remote/init.rs @@ -44,7 +44,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(|name| remote::name::validated(name)).transpose()?, + name: name.map(remote::name::validated).transpose()?, url, url_alias, push_url, From f993cd4ca03a22d27c85ff229bab66adcb4e493a Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 10:48:32 +0800 Subject: [PATCH 31/42] sketch and test for `clone::Prepare::fetch_only()` --- git-repository/src/clone.rs | 55 ++++++++++++++++++++++++++++--- git-repository/tests/clone/mod.rs | 38 ++++++++++++++++++--- 2 files changed, 85 insertions(+), 8 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 1361dd6ea7e..b5303b06bd4 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -1,4 +1,4 @@ -type ConfigureRemoteFn = Box) -> Result, crate::remote::init::Error>>; +type ConfigureRemoteFn = Box) -> Result, crate::remote::init::Error>>; /// A utility to collect configuration on how to fetch from a remote and possibly create a working tree locally. pub struct Prepare { @@ -8,6 +8,9 @@ pub struct Prepare { remote_name: Option, /// A function to configure a remote prior to fetching a pack. configure_remote: Option, + /// Options for preparing a fetch operation. + #[cfg(any(feature = "async-network-client", feature = "blocking-network-client"))] + fetch_options: crate::remote::ref_map::Options, /// The url to clone from #[allow(dead_code)] url: git_url::Url, @@ -29,6 +32,24 @@ pub mod prepare { UrlParse(#[from] git_url::parse::Error), } + /// + #[cfg(feature = "blocking-network-client")] + pub mod fetch { + /// The error returned by [`Prepare::fetch_only()`]. + #[derive(Debug, thiserror::Error)] + #[allow(missing_docs)] + pub enum Error { + #[error(transparent)] + Connect(#[from] crate::remote::connect::Error), + #[error(transparent)] + PrepareFetch(#[from] crate::remote::fetch::prepare::Error), + #[error(transparent)] + Fetch(#[from] crate::remote::fetch::Error), + #[error(transparent)] + RemoteConfiguration(#[from] crate::remote::init::Error), + } + } + /// Instantiation impl Prepare { /// Create a new repository at `path` with `crate_opts` which is ready to clone from `url`, possibly after making additional adjustments to @@ -50,6 +71,8 @@ pub mod prepare { let repo = crate::ThreadSafeRepository::init_opts(path, create_opts, open_opts)?.to_thread_local(); Ok(Prepare { url, + #[cfg(any(feature = "async-network-client", feature = "blocking-network-client"))] + fetch_options: Default::default(), repo: Some(repo), remote_name: None, configure_remote: None, @@ -57,8 +80,32 @@ pub mod prepare { } } + /// Modification + impl Prepare { + /// Fetch a pack and update local branches according to refspecs, providing `progress` and checking `should_interrupt` to stop + /// the operation. + /// On success, the persisted repository is returned, and this method must not be called again to avoid a **panic**. + /// On error, the method may be called again to retry as often as needed. + /// + /// Note that all data we created will be removed once this instance drops if the operation wasn't successful. + #[cfg(feature = "blocking-network-client")] + pub fn fetch_only( + &mut self, + progress: impl crate::Progress, + should_interrupt: &std::sync::atomic::AtomicBool, + ) -> Result { + todo!() + } + } + /// Builder impl Prepare { + /// Set additional options to adjust parts of the fetch operation that are not affected by the git configuration. + #[cfg(any(feature = "async-network-client", feature = "blocking-network-client"))] + pub fn with_fetch_options(mut self, opts: crate::remote::ref_map::Options) -> Self { + self.fetch_options = opts; + self + } /// Use `f` to apply arbitrary changes to the remote that is about to be used to fetch a pack. /// /// The passed in `remote` will be un-named and pre-configured to be a default remote as we know it from git-clone. @@ -66,7 +113,7 @@ pub mod prepare { /// but each change it will eventually be written to the configuration prior to performing a the fetch operation. pub fn configure_remote( mut self, - f: impl FnOnce(crate::Remote<'_>) -> Result, crate::remote::init::Error> + 'static, + f: impl FnMut(crate::Remote<'_>) -> Result, crate::remote::init::Error> + 'static, ) -> Self { self.configure_remote = Some(Box::new(f)); self @@ -76,13 +123,13 @@ pub mod prepare { /// [`configure_remote()`][Self::configure_remote()]. /// /// If not set here, it defaults to `origin`. - pub fn 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) } } - /// Access + /// Consumption impl Prepare { /// Persist the contained repository as is even if an error may have occurred when interacting with the remote or checking out the main working tree. pub fn persist(mut self) -> Repository { diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 152f0449717..e662697734d 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -2,7 +2,38 @@ use crate::remote; use git_repository as git; #[test] -fn persist() -> crate::Result { +#[cfg(feature = "blocking-network-client")] +fn fetch_only_with_configuration() -> crate::Result { + let tmp = git_testtools::tempfile::TempDir::new()?; + let mut called_configure_remote = false; + let remote_name = "special"; + let mut prepare = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())? + .with_remote_name(remote_name)? + .configure_remote(move |r| { + // TODO: no move + called_configure_remote = true; + Ok( + r.with_refspec("+refs/tags/*:refs/tags/*", git::remote::Direction::Fetch) + .expect("valid static spec"), + ) + }); + let repo = prepare.fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; + drop(prepare); + + assert_eq!(called_configure_remote, true, "custom remote configuration is called"); + assert_eq!(repo.remote_names().len(), 1, "only ever one remote"); + let remote = repo.find_remote(remote_name)?; + assert_eq!( + remote.refspecs(git::remote::Direction::Fetch).len(), + 2, + "our added spec was stored as well" + ); + + Ok(()) +} + +#[test] +fn clone_and_early_persist_without_receive() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; let repo = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())?.persist(); assert!(repo.is_bare(), "repo is now ours and remains"); @@ -10,7 +41,7 @@ fn persist() -> crate::Result { } #[test] -fn clone_bare_into_empty_directory() -> crate::Result { +fn clone_bare_into_empty_directory_and_early_drop() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; // this breaks isolation, but shouldn't be affecting the test. If so, use isolation options for opening the repo. let prep = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())?; @@ -23,9 +54,8 @@ fn clone_bare_into_empty_directory() -> crate::Result { } #[test] -fn clone_into_empty_directory() -> crate::Result { +fn clone_into_empty_directory_and_early_drop() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; - // this breaks isolation, but shouldn't be affecting the test. If so, use isolation options for opening the repo. let prep = git::prepare_clone(remote::repo("base").path(), tmp.path())?; let head = tmp.path().join(".git").join("HEAD"); assert!(head.is_file(), "now a basic repo is present"); From 8b804a31cb20a5264311f0b6ba02f857bea933ad Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 11:08:00 +0800 Subject: [PATCH 32/42] update progress with intended uses of `clone.` variables --- src/plumbing/progress.rs | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index eddd738a07c..e39e26afc22 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -10,6 +10,9 @@ enum Usage { Planned { note: Option<&'static str>, }, + NotPlanned { + reason: &'static str, + }, InModule { name: &'static str, deviation: Option<&'static str>, @@ -24,6 +27,10 @@ impl Display for Usage { match self { Puzzled => f.write_str("❓")?, NotApplicable => f.write_str("not applicable")?, + NotPlanned { reason } => { + write!(f, "{}", "not planned".blink())?; + write!(f, " ℹ {} ℹ", reason.bright_white())?; + } Planned { note } => { write!(f, "{}", "planned".blink())?; if let Some(note) = note { @@ -47,6 +54,7 @@ impl Usage { Puzzled => "?", NotApplicable => "❌", Planned { .. } => "🕒", + NotPlanned { .. } => "🤔", InModule { deviation, .. } => deviation.is_some().then(|| "👌️").unwrap_or("✅"), } } @@ -190,6 +198,24 @@ static GIT_CONFIG: &[Record] = &[ deviation: Some("defaults to 'gitoxide@localhost'"), }, }, + Record { + config: "clone.filterSubmodules,", + usage: Planned { + note: Some("currently object filtering isn't support, a prerequisite for this, see --filter=blob:none for more"), + }, + }, + Record { + config: "clone.defaultRemoteName", + usage: Planned { + note: None, + }, + }, + Record { + config: "clone.rejectShallow", + usage: NotPlanned { + reason: "it's not a use-case we consider important now, but once that changes it can be implemented", + }, + }, Record { config: "fetch.recurseSubmodules", usage: Planned { From b0f5836c53fc95c408dac4b6e370532695373063 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 12:04:22 +0800 Subject: [PATCH 33/42] Complete implementation of `fetch_only`, even though configuration isn't handled correctly yet --- git-repository/src/clone.rs | 49 ++++++++++++++++++- .../src/remote/connection/fetch/mod.rs | 11 +++++ git-repository/tests/clone/mod.rs | 25 ++++++---- 3 files changed, 74 insertions(+), 11 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index b5303b06bd4..f647f9ce7df 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -47,6 +47,14 @@ pub mod prepare { Fetch(#[from] crate::remote::fetch::Error), #[error(transparent)] RemoteConfiguration(#[from] crate::remote::init::Error), + #[error("Default remote configured at `clone.defaultRemoteName` is invalid")] + RemoteName(#[from] crate::remote::name::Error), + #[error("Failed to load repo-local git configuration before writing")] + LoadConfig(#[from] git_config::file::init::from_paths::Error), + #[error("Failed to store configured remote in memory")] + SaveConfig(#[from] crate::remote::save::AsError), + #[error("Failed to write repository configuration to disk")] + SaveConfigIo(#[from] std::io::Error), } } @@ -94,7 +102,44 @@ pub mod prepare { progress: impl crate::Progress, should_interrupt: &std::sync::atomic::AtomicBool, ) -> Result { - todo!() + // TODO: provide fetch::Outcome somehow - it references `Repository`. + let repo = self + .repo + .as_ref() + .expect("user error: multiple calls are allowed only until it succeeds"); + + let remote_name = match self.remote_name.as_deref() { + Some(name) => name.to_owned(), + None => repo + .config + .resolved + .string("clone", None, "defaultRemoteName") + .map(|n| crate::remote::name::validated(n.to_string())) + .unwrap_or_else(|| Ok("origin".into()))?, + }; + + let mut remote = repo + .remote_at(self.url.clone())? + .with_refspec("+refs/heads/*:refs/remotes/origin/*", crate::remote::Direction::Fetch) + .expect("valid static spec"); + if let Some(f) = self.configure_remote.as_mut() { + remote = f(remote)?; + } + + let mut metadata = git_config::file::Metadata::from(git_config::Source::Local); + let config_path = repo.git_dir().join("config"); + metadata.path = Some(config_path.clone()); + let mut config = + git_config::File::from_paths_metadata(Some(metadata), Default::default())?.expect("one file to load"); + remote.save_as_to(remote_name, &mut config)?; + std::fs::write(config_path, config.to_bstring())?; + + remote + .connect(crate::remote::Direction::Fetch, progress)? + .prepare_fetch(self.fetch_options.clone())? + .receive(should_interrupt)?; + + Ok(self.repo.take().expect("still present")) } } @@ -122,7 +167,7 @@ pub mod prepare { /// Set the remote's name to the given value after it was configured using the function provided via /// [`configure_remote()`][Self::configure_remote()]. /// - /// If not set here, it defaults to `origin`. + /// 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 { self.remote_name = Some(crate::remote::name::validated(name)?); Ok(self) diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index 8f8d3e104a9..6f94b5c89b7 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -125,6 +125,17 @@ where /// /// "fetch.negotiationAlgorithm" describes algorithms `git` uses currently, with the default being `consecutive` and `skipping` being /// experimented with. We currently implement something we could call 'naive' which works for now. + /// + /// + /// ### Deviation + /// + /// When **updating refs**, the `git-fetch` docs state that the following: + /// + /// > Unlike when pushing with git-push, any updates outside of refs/{tags,heads}/* will be accepted without + in the refspec (or --force), whether that’s swapping e.g. a tree object for a blob, or a commit for another commit that’s doesn’t have the previous commit as an ancestor etc. + /// + /// We explicitly don't special case those refs and expect the user to take control. Note that by its nature, + /// force only applies to refs pointing to commits and if they don't, they will be updated either way in our + /// implementation as well. pub fn receive(mut self, should_interrupt: &AtomicBool) -> Result, Error> { let mut con = self.con.take().expect("receive() can only be called once"); diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index e662697734d..1b1184b80b1 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -3,24 +3,31 @@ use git_repository as git; #[test] #[cfg(feature = "blocking-network-client")] +#[ignore] fn fetch_only_with_configuration() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; - let mut called_configure_remote = false; + let called_configure_remote = std::sync::Arc::new(std::sync::atomic::AtomicBool::default()); let remote_name = "special"; let mut prepare = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())? .with_remote_name(remote_name)? - .configure_remote(move |r| { - // TODO: no move - called_configure_remote = true; - Ok( - r.with_refspec("+refs/tags/*:refs/tags/*", git::remote::Direction::Fetch) - .expect("valid static spec"), - ) + .configure_remote({ + let called_configure_remote = called_configure_remote.clone(); + move |r| { + called_configure_remote.store(true, std::sync::atomic::Ordering::Relaxed); + Ok( + r.with_refspec("+refs/tags/*:refs/tags/*", git::remote::Direction::Fetch) + .expect("valid static spec"), + ) + } }); let repo = prepare.fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; drop(prepare); - assert_eq!(called_configure_remote, true, "custom remote configuration is called"); + assert_eq!( + called_configure_remote.load(std::sync::atomic::Ordering::Relaxed), + true, + "custom remote configuration is called" + ); assert_eq!(repo.remote_names().len(), 1, "only ever one remote"); let remote = repo.find_remote(remote_name)?; assert_eq!( From 5ecc965b8804500c537acdac2f1d8751324f4736 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 12:12:44 +0800 Subject: [PATCH 34/42] the first successful test for cloning a bare repository --- git-repository/src/clone.rs | 13 ++++++++++++- git-repository/tests/clone/mod.rs | 1 - 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index f647f9ce7df..2057b1c7a2d 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -20,6 +20,7 @@ pub struct Prepare { pub mod prepare { use crate::clone::Prepare; use crate::Repository; + use git_features::threading::OwnShared; use std::convert::TryInto; /// The error returned by [`Prepare::new()`]. @@ -105,7 +106,7 @@ pub mod prepare { // TODO: provide fetch::Outcome somehow - it references `Repository`. let repo = self .repo - .as_ref() + .as_mut() .expect("user error: multiple calls are allowed only until it succeeds"); let remote_name = match self.remote_name.as_deref() { @@ -139,6 +140,16 @@ pub mod prepare { .prepare_fetch(self.fetch_options.clone())? .receive(should_interrupt)?; + let repo_config = OwnShared::make_mut(&mut repo.config.resolved); + let ids_to_remove: Vec<_> = repo_config + .sections_and_ids() + .filter_map(|(s, id)| (s.meta().source == git_config::Source::Local).then(|| id)) + .collect(); + for id in ids_to_remove { + repo_config.remove_section_by_id(id); + } + repo_config.append(config); + Ok(self.repo.take().expect("still present")) } } diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 1b1184b80b1..80c2b95b052 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -3,7 +3,6 @@ use git_repository as git; #[test] #[cfg(feature = "blocking-network-client")] -#[ignore] fn fetch_only_with_configuration() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; let called_configure_remote = std::sync::Arc::new(std::sync::atomic::AtomicBool::default()); From 1a1e862b2e8cd88f5f6fbb9d86f618761bb71ef1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 12:13:09 +0800 Subject: [PATCH 35/42] update usage of clone related configuration --- src/plumbing/progress.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index e39e26afc22..a6ae7be2afc 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -206,8 +206,9 @@ static GIT_CONFIG: &[Record] = &[ }, Record { config: "clone.defaultRemoteName", - usage: Planned { - note: None, + usage: InModule { + name: "clone::prepare", + deviation: None }, }, Record { From bc2710f1833d3f3423868bd4c7d8df991200b39d Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 12:13:53 +0800 Subject: [PATCH 36/42] thanks clippy --- git-repository/src/clone.rs | 3 +-- git-repository/tests/clone/mod.rs | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 2057b1c7a2d..1a5891466da 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -20,7 +20,6 @@ pub struct Prepare { pub mod prepare { use crate::clone::Prepare; use crate::Repository; - use git_features::threading::OwnShared; use std::convert::TryInto; /// The error returned by [`Prepare::new()`]. @@ -140,7 +139,7 @@ pub mod prepare { .prepare_fetch(self.fetch_options.clone())? .receive(should_interrupt)?; - let repo_config = OwnShared::make_mut(&mut repo.config.resolved); + let repo_config = git_features::threading::OwnShared::make_mut(&mut repo.config.resolved); let ids_to_remove: Vec<_> = repo_config .sections_and_ids() .filter_map(|(s, id)| (s.meta().source == git_config::Source::Local).then(|| id)) diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 80c2b95b052..77d053a6f03 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -22,9 +22,8 @@ fn fetch_only_with_configuration() -> crate::Result { let repo = prepare.fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; drop(prepare); - assert_eq!( + assert!( called_configure_remote.load(std::sync::atomic::Ordering::Relaxed), - true, "custom remote configuration is called" ); assert_eq!(repo.remote_names().len(), 1, "only ever one remote"); From 2a0a87a04e7b4d6ed3be3d8adc89917576727686 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 14:15:27 +0800 Subject: [PATCH 37/42] change!: remove lifetime of `match_group::Fix`, keeping `RefSpec` instances instead That lifetime unnecessarily complicated things and wasn't worth keeping due to being a premature optimization. --- git-refspec/src/match_group/validate.rs | 10 +++++----- git-refspec/tests/match_group/mod.rs | 6 +++--- git-refspec/tests/matching/mod.rs | 15 +++++---------- 3 files changed, 13 insertions(+), 18 deletions(-) diff --git a/git-refspec/src/match_group/validate.rs b/git-refspec/src/match_group/validate.rs index c1eab28e5e1..783ef4c5d15 100644 --- a/git-refspec/src/match_group/validate.rs +++ b/git-refspec/src/match_group/validate.rs @@ -4,7 +4,7 @@ use bstr::BString; use crate::{ match_group::{Outcome, Source}, - RefSpecRef, + RefSpec, }; /// All possible issues found while validating matched mappings. @@ -49,13 +49,13 @@ impl std::fmt::Display for Issue { /// All possible fixes corrected while validating matched mappings. #[derive(Debug, PartialEq, Eq, Clone)] -pub enum Fix<'a> { +pub enum Fix { /// Removed a mapping that contained a partial destination entirely. MappingWithPartialDestinationRemoved { /// The destination ref name that was ignored. name: BString, /// The spec that defined the mapping - spec: RefSpecRef<'a>, + spec: RefSpec, }, } @@ -91,7 +91,7 @@ impl<'spec, 'item> Outcome<'spec, 'item> { /// Return `(modified self, issues)` providing a fixed-up set of mappings in `self` with the fixed `issues` /// provided as part of it. /// Terminal issues are communicated using the [`Error`] type accordingly. - pub fn validated(mut self) -> Result<(Self, Vec>), Error> { + pub fn validated(mut self) -> Result<(Self, Vec), Error> { let mut sources_by_destinations = BTreeMap::new(); for (dst, (spec_index, src)) in self .mappings @@ -126,7 +126,7 @@ impl<'spec, 'item> Outcome<'spec, 'item> { } else { fixed.push(Fix::MappingWithPartialDestinationRemoved { name: dst.as_ref().to_owned(), - spec: group.specs[m.spec_index], + spec: group.specs[m.spec_index].to_owned(), }); false } diff --git a/git-refspec/tests/match_group/mod.rs b/git-refspec/tests/match_group/mod.rs index 0b11a5c61df..565f533c5e7 100644 --- a/git-refspec/tests/match_group/mod.rs +++ b/git-refspec/tests/match_group/mod.rs @@ -169,15 +169,15 @@ mod multiple { [ Fix::MappingWithPartialDestinationRemoved { name: "foo/f1".into(), - spec: glob_spec_ref, + spec: glob_spec_ref.to_owned(), }, Fix::MappingWithPartialDestinationRemoved { name: "foo/f2".into(), - spec: glob_spec_ref, + spec: glob_spec_ref.to_owned(), }, Fix::MappingWithPartialDestinationRemoved { name: "foo/f3".into(), - spec: glob_spec_ref, + spec: glob_spec_ref.to_owned(), }, ], ["refs/heads/f1:refs/heads/f1"], diff --git a/git-refspec/tests/matching/mod.rs b/git-refspec/tests/matching/mod.rs index 12a086b76be..26cd5808820 100644 --- a/git-refspec/tests/matching/mod.rs +++ b/git-refspec/tests/matching/mod.rs @@ -56,9 +56,9 @@ pub mod baseline { agrees_and_applies_fixes(specs, Vec::new(), expected) } - pub fn agrees_and_applies_fixes<'a, 'b, 'c>( + pub fn agrees_and_applies_fixes<'a, 'b>( specs: impl IntoIterator + Clone, - fixes: impl IntoIterator>, + fixes: impl IntoIterator, expected: impl IntoIterator, ) { check_fetch_remote( @@ -127,14 +127,9 @@ pub mod baseline { of_objects_with_destinations_are_written_into_given_local_branches(specs, expected) } - enum Mode<'a> { - Normal { - validate_err: Option, - }, - Custom { - expected: Vec, - fixes: Vec>, - }, + enum Mode { + Normal { validate_err: Option }, + Custom { expected: Vec, fixes: Vec }, } fn check_fetch_remote<'a>(specs: impl IntoIterator + Clone, mode: Mode) { From c62f37a4e3599049d10e3d7534a553e8efa2dcd7 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 14:15:39 +0800 Subject: [PATCH 38/42] adjust to changes in `git-refspec` --- git-repository/src/remote/connection/fetch/mod.rs | 8 ++++---- git-repository/src/remote/connection/fetch/negotiate.rs | 2 +- git-repository/src/remote/connection/ref_map.rs | 4 ++-- git-repository/src/remote/fetch.rs | 4 ++-- git-repository/tests/remote/fetch.rs | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/git-repository/src/remote/connection/fetch/mod.rs b/git-repository/src/remote/connection/fetch/mod.rs index 6f94b5c89b7..e56309abf31 100644 --- a/git-repository/src/remote/connection/fetch/mod.rs +++ b/git-repository/src/remote/connection/fetch/mod.rs @@ -60,9 +60,9 @@ pub enum Status { /// The outcome of receiving a pack via [`Prepare::receive()`]. #[derive(Debug, Clone)] -pub struct Outcome<'spec> { +pub struct Outcome { /// The result of the initial mapping of references, the prerequisite for any fetch. - pub ref_map: RefMap<'spec>, + pub ref_map: RefMap, /// The status of the operation to indicate what happened. pub status: Status, } @@ -136,7 +136,7 @@ where /// We explicitly don't special case those refs and expect the user to take control. Note that by its nature, /// force only applies to refs pointing to commits and if they don't, they will be updated either way in our /// implementation as well. - pub fn receive(mut self, should_interrupt: &AtomicBool) -> Result, Error> { + pub fn receive(mut self, should_interrupt: &AtomicBool) -> Result { let mut con = self.con.take().expect("receive() can only be called once"); let handshake = &self.ref_map.handshake; @@ -269,7 +269,7 @@ where T: Transport, { con: Option>, - ref_map: RefMap<'remote>, + ref_map: RefMap, dry_run: DryRun, } diff --git a/git-repository/src/remote/connection/fetch/negotiate.rs b/git-repository/src/remote/connection/fetch/negotiate.rs index f020732b74d..51bb25ff294 100644 --- a/git-repository/src/remote/connection/fetch/negotiate.rs +++ b/git-repository/src/remote/connection/fetch/negotiate.rs @@ -20,7 +20,7 @@ pub(crate) fn one_round( algo: Algorithm, round: usize, repo: &crate::Repository, - ref_map: &crate::remote::fetch::RefMap<'_>, + ref_map: &crate::remote::fetch::RefMap, arguments: &mut git_protocol::fetch::Arguments, _previous_response: Option<&git_protocol::fetch::Response>, ) -> Result { diff --git a/git-repository/src/remote/connection/ref_map.rs b/git-repository/src/remote/connection/ref_map.rs index e231f783218..fc43a332096 100644 --- a/git-repository/src/remote/connection/ref_map.rs +++ b/git-repository/src/remote/connection/ref_map.rs @@ -63,7 +63,7 @@ where /// Due to management of the transport, it's cleanest to only use it for a single interaction. Thus it's consumed along with /// the connection. #[git_protocol::maybe_async::maybe_async] - pub async fn ref_map(mut self, options: Options) -> Result, Error> { + pub async fn ref_map(mut self, options: Options) -> Result { let res = self.ref_map_inner(options).await; git_protocol::fetch::indicate_end_of_interaction(&mut self.transport) .await @@ -78,7 +78,7 @@ where prefix_from_spec_as_filter_on_remote, handshake_parameters, }: Options, - ) -> Result, Error> { + ) -> Result { let remote = self .fetch_refs(prefix_from_spec_as_filter_on_remote, handshake_parameters) .await?; diff --git a/git-repository/src/remote/fetch.rs b/git-repository/src/remote/fetch.rs index bc528969726..b23cad887fe 100644 --- a/git-repository/src/remote/fetch.rs +++ b/git-repository/src/remote/fetch.rs @@ -12,11 +12,11 @@ pub(crate) enum DryRun { /// Information about the relationship between our refspecs, and remote references with their local counterparts. #[derive(Default, Debug, Clone)] -pub struct RefMap<'spec> { +pub struct RefMap { /// A mapping between a remote reference and a local tracking branch. pub mappings: Vec, /// Information about the fixes applied to the `mapping` due to validation and sanitization. - pub fixes: Vec>, + pub fixes: Vec, /// All refs advertised by the remote. pub remote_refs: Vec, /// Additional information provided by the server as part of the handshake. diff --git a/git-repository/tests/remote/fetch.rs b/git-repository/tests/remote/fetch.rs index 9a0cfabfad7..32689098962 100644 --- a/git-repository/tests/remote/fetch.rs +++ b/git-repository/tests/remote/fetch.rs @@ -28,7 +28,7 @@ mod blocking_io { let mut remote = repo.head()?.into_remote(Fetch).expect("present")?; remote.replace_refspecs(Some("HEAD:refs/remotes/origin/does-not-exist"), Fetch)?; - let res: git::remote::fetch::Outcome<'_> = remote + let res: git::remote::fetch::Outcome = remote .connect(Fetch, git::progress::Discard)? .prepare_fetch(Default::default())? .receive(&AtomicBool::default())?; From 3865aea50b05b4a28caf0d364fea4876c393f976 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 14:27:00 +0800 Subject: [PATCH 39/42] validate the outcome of the fetch as well --- git-repository/src/clone.rs | 7 +++---- git-repository/tests/clone/mod.rs | 16 +++++++++++++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 1a5891466da..3ab48354557 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -101,8 +101,7 @@ pub mod prepare { &mut self, progress: impl crate::Progress, should_interrupt: &std::sync::atomic::AtomicBool, - ) -> Result { - // TODO: provide fetch::Outcome somehow - it references `Repository`. + ) -> Result<(Repository, crate::remote::fetch::Outcome), fetch::Error> { let repo = self .repo .as_mut() @@ -134,7 +133,7 @@ pub mod prepare { remote.save_as_to(remote_name, &mut config)?; std::fs::write(config_path, config.to_bstring())?; - remote + let outcome = remote .connect(crate::remote::Direction::Fetch, progress)? .prepare_fetch(self.fetch_options.clone())? .receive(should_interrupt)?; @@ -149,7 +148,7 @@ pub mod prepare { } repo_config.append(config); - Ok(self.repo.take().expect("still present")) + Ok((self.repo.take().expect("still present"), outcome)) } } diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index 77d053a6f03..e3cafbd5752 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -1,5 +1,7 @@ use crate::remote; +use git_odb::Find; use git_repository as git; +use git_repository::remote::fetch::Status; #[test] #[cfg(feature = "blocking-network-client")] @@ -19,7 +21,7 @@ fn fetch_only_with_configuration() -> crate::Result { ) } }); - let repo = prepare.fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; + let (repo, out) = prepare.fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; drop(prepare); assert!( @@ -34,6 +36,18 @@ fn fetch_only_with_configuration() -> crate::Result { "our added spec was stored as well" ); + assert_eq!(out.ref_map.mappings.len(), 13); + match out.status { + Status::Change { update_refs, .. } => { + for edit in &update_refs.edits { + assert!( + repo.objects.contains(edit.change.new_value().expect("always set").id()), + "part of the fetched pack" + ); + } + } + _ => unreachable!("clones are always causing changes and dry-runs aren't possible"), + } Ok(()) } From dff66e898bb3cc033c85464ccd937b1a0e891750 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 14:29:20 +0800 Subject: [PATCH 40/42] validate default remote name as well and show minimal clone without worktree --- git-repository/tests/clone/mod.rs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index e3cafbd5752..ad0b3ccce79 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -1,7 +1,5 @@ use crate::remote; -use git_odb::Find; use git_repository as git; -use git_repository::remote::fetch::Status; #[test] #[cfg(feature = "blocking-network-client")] @@ -38,8 +36,9 @@ fn fetch_only_with_configuration() -> crate::Result { assert_eq!(out.ref_map.mappings.len(), 13); match out.status { - Status::Change { update_refs, .. } => { + git_repository::remote::fetch::Status::Change { update_refs, .. } => { for edit in &update_refs.edits { + use git_odb::Find; assert!( repo.objects.contains(edit.change.new_value().expect("always set").id()), "part of the fetched pack" @@ -51,6 +50,16 @@ fn fetch_only_with_configuration() -> crate::Result { Ok(()) } +#[test] +#[cfg(feature = "blocking-network-client")] +fn fetch_only_without_configuration() -> crate::Result { + let tmp = git_testtools::tempfile::TempDir::new()?; + let (repo, _out) = git::prepare_clone_bare(remote::repo("base").path(), tmp.path())? + .fetch_only(git::progress::Discard, &std::sync::atomic::AtomicBool::default())?; + assert!(repo.find_remote("origin").is_ok(), "default remote name is 'origin'"); + Ok(()) +} + #[test] fn clone_and_early_persist_without_receive() -> crate::Result { let tmp = git_testtools::tempfile::TempDir::new()?; From 314b5e0b39f1aec3f577ab67fdde7dd6dc487c0a Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 14:42:47 +0800 Subject: [PATCH 41/42] fix build --- gitoxide-core/src/repository/fetch.rs | 13 ++++++++----- gitoxide-core/src/repository/remote.rs | 11 +++++++---- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/gitoxide-core/src/repository/fetch.rs b/gitoxide-core/src/repository/fetch.rs index bfe6096c26f..0fe450845f8 100644 --- a/gitoxide-core/src/repository/fetch.rs +++ b/gitoxide-core/src/repository/fetch.rs @@ -43,7 +43,7 @@ pub(crate) mod function { if !ref_specs.is_empty() { remote.replace_refspecs(ref_specs.iter(), git::remote::Direction::Fetch)?; } - let res: git::remote::fetch::Outcome<'_> = remote + let res: git::remote::fetch::Outcome = remote .connect(git::remote::Direction::Fetch, progress)? .prepare_fetch(Default::default())? .with_dry_run(dry_run) @@ -84,7 +84,7 @@ pub(crate) mod function { repo: &git::Repository, update_refs: git::remote::fetch::refs::update::Outcome, refspecs: &[git::refspec::RefSpec], - mut map: git::remote::fetch::RefMap<'_>, + mut map: git::remote::fetch::RefMap, mut out: impl std::io::Write, mut err: impl std::io::Write, ) -> anyhow::Result<()> { @@ -121,8 +121,11 @@ pub(crate) mod function { err, "The following destination refs were removed as they didn't start with 'ref/'" )?; - map.fixes.sort_by_key(|f| match f { - Fix::MappingWithPartialDestinationRemoved { spec, .. } => *spec, + map.fixes.sort_by(|l, r| match (l, r) { + ( + Fix::MappingWithPartialDestinationRemoved { spec: l, .. }, + Fix::MappingWithPartialDestinationRemoved { spec: r, .. }, + ) => l.cmp(&r), }); let mut prev_spec = None; for fix in &map.fixes { @@ -130,7 +133,7 @@ pub(crate) mod function { Fix::MappingWithPartialDestinationRemoved { name, spec } => { if prev_spec.map_or(true, |prev_spec| prev_spec != spec) { prev_spec = spec.into(); - spec.write_to(&mut err)?; + spec.to_ref().write_to(&mut err)?; writeln!(err)?; } writeln!(err, "\t{name}")?; diff --git a/gitoxide-core/src/repository/remote.rs b/gitoxide-core/src/repository/remote.rs index 6bbbd53de3b..2543b6103e7 100644 --- a/gitoxide-core/src/repository/remote.rs +++ b/gitoxide-core/src/repository/remote.rs @@ -95,7 +95,7 @@ mod refs_impl { pub(crate) fn print_refmap( repo: &git::Repository, refspecs: &[RefSpec], - mut map: git::remote::fetch::RefMap<'_>, + mut map: git::remote::fetch::RefMap, mut out: impl std::io::Write, mut err: impl std::io::Write, ) -> anyhow::Result<()> { @@ -141,8 +141,11 @@ mod refs_impl { err, "The following destination refs were removed as they didn't start with 'ref/'" )?; - map.fixes.sort_by_key(|f| match f { - Fix::MappingWithPartialDestinationRemoved { spec, .. } => *spec, + map.fixes.sort_by(|l, r| match (l, r) { + ( + Fix::MappingWithPartialDestinationRemoved { spec: l, .. }, + Fix::MappingWithPartialDestinationRemoved { spec: r, .. }, + ) => l.cmp(&r), }); let mut prev_spec = None; for fix in &map.fixes { @@ -150,7 +153,7 @@ mod refs_impl { Fix::MappingWithPartialDestinationRemoved { name, spec } => { if prev_spec.map_or(true, |prev_spec| prev_spec != spec) { prev_spec = spec.into(); - spec.write_to(&mut err)?; + spec.to_ref().write_to(&mut err)?; writeln!(err)?; } writeln!(err, "\t{name}")?; From 44743524fd35c7987fee2f46d134e4b0e328e133 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 10 Oct 2022 14:44:33 +0800 Subject: [PATCH 42/42] fix docs --- git-repository/src/clone.rs | 2 +- git-repository/src/remote/mod.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/git-repository/src/clone.rs b/git-repository/src/clone.rs index 3ab48354557..256f5b2e916 100644 --- a/git-repository/src/clone.rs +++ b/git-repository/src/clone.rs @@ -35,7 +35,7 @@ pub mod prepare { /// #[cfg(feature = "blocking-network-client")] pub mod fetch { - /// The error returned by [`Prepare::fetch_only()`]. + /// The error returned by [`Prepare::fetch_only()`][super::Prepare::fetch_only()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { diff --git a/git-repository/src/remote/mod.rs b/git-repository/src/remote/mod.rs index 1330db8d043..809b38c6a4a 100644 --- a/git-repository/src/remote/mod.rs +++ b/git-repository/src/remote/mod.rs @@ -24,7 +24,7 @@ pub use errors::find; /// pub mod name { - /// The error returned by [validate()]. + /// The error returned by [validated()]. #[derive(Debug, thiserror::Error)] #[error("remote names must be valid within refspecs for fetching: {name:?}")] #[allow(missing_docs)]