From 25e795f4fe858d646ae7a3c4706e14a3837c3e66 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 11:29:15 +0100 Subject: [PATCH 01/29] feat: Add `os_string_into_bstring()` as sibling of `os_str_into_bstr()`. --- git-path/src/convert.rs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/git-path/src/convert.rs b/git-path/src/convert.rs index 81701438cfb..e33254e9712 100644 --- a/git-path/src/convert.rs +++ b/git-path/src/convert.rs @@ -1,3 +1,4 @@ +use std::ffi::OsString; use std::{ borrow::Cow, ffi::OsStr, @@ -28,6 +29,15 @@ pub fn os_str_into_bstr(path: &OsStr) -> Result<&BStr, Utf8Error> { } } +/// Like [`into_bstr()`], but takes `OsString` as input for a lossless, but fallible, conversion. +pub fn os_string_into_bstring(path: OsString) -> Result { + let path = try_into_bstr(Cow::Owned(path.into()))?; + match path { + Cow::Borrowed(_path) => unreachable!("borrowed cows stay borrowed"), + Cow::Owned(path) => Ok(path), + } +} + /// Convert the given path either into its raw bytes on unix or its UTF8 encoded counterpart on windows. /// /// On windows, if the source Path contains ill-formed, lone surrogates, the UTF-8 conversion will fail From 0c98ec8fc7d8cc3195472a04fde4a681f620725f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 10:22:14 +0100 Subject: [PATCH 02/29] fix!: subsections are identified as `&BStr` in entire API. Technically they can be any value (except for newlines and unescaped double quotes), and these values might be paths and everything that comes with it, like illformed UTF8. In order to be able to represent everything that git can represent, we don't enforce UTF8 anymore for subsection names. Note that section names and key names are required to be valid UTF8 (and even alphanumeric ascii), which makes illformed UTF8 very unlikely there. --- git-config/src/file/access/comfort.rs | 24 +++++++-------- git-config/src/file/access/mutate.rs | 29 ++++++++++--------- git-config/src/file/access/raw.rs | 24 +++++++-------- git-config/src/file/access/read_only.rs | 10 +++---- git-config/src/file/init/from_env.rs | 15 ++++++++-- git-config/src/file/section/mod.rs | 4 +-- git-config/src/file/util.rs | 2 +- git-config/src/parse/key.rs | 24 ++++++++++----- git-config/src/parse/nom/tests.rs | 18 ++++++++++++ git-config/src/parse/section/header.rs | 6 ++-- git-config/tests/file/access/mutate.rs | 6 ++-- .../tests/file/access/raw/raw_multi_value.rs | 4 +-- git-config/tests/file/access/raw/raw_value.rs | 4 +-- .../tests/file/access/raw/set_raw_value.rs | 6 ++-- git-config/tests/file/access/read_only.rs | 2 +- git-config/tests/file/mutable/section.rs | 4 +-- git-config/tests/parse/key.rs | 4 +-- git-config/tests/parse/section.rs | 24 ++++++++------- 18 files changed, 127 insertions(+), 83 deletions(-) diff --git a/git-config/src/file/access/comfort.rs b/git-config/src/file/access/comfort.rs index ad334c7dff8..c81557fbb44 100644 --- a/git-config/src/file/access/comfort.rs +++ b/git-config/src/file/access/comfort.rs @@ -12,7 +12,7 @@ impl<'event> File<'event> { pub fn string( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option> { self.string_filter(section_name, subsection_name, key, &mut |_| true) @@ -22,7 +22,7 @@ impl<'event> File<'event> { pub fn string_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Option> { @@ -38,7 +38,7 @@ impl<'event> File<'event> { pub fn path( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option> { self.path_filter(section_name, subsection_name, key, &mut |_| true) @@ -53,7 +53,7 @@ impl<'event> File<'event> { pub fn path_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Option> { @@ -66,7 +66,7 @@ impl<'event> File<'event> { pub fn boolean( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option> { self.boolean_filter(section_name, subsection_name, key, &mut |_| true) @@ -76,7 +76,7 @@ impl<'event> File<'event> { pub fn boolean_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Option> { @@ -103,7 +103,7 @@ impl<'event> File<'event> { pub fn integer( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option> { self.integer_filter(section_name, subsection_name, key, &mut |_| true) @@ -113,7 +113,7 @@ impl<'event> File<'event> { pub fn integer_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Option> { @@ -128,7 +128,7 @@ impl<'event> File<'event> { pub fn strings( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option>> { self.raw_values(section_name, subsection_name, key).ok() @@ -138,7 +138,7 @@ impl<'event> File<'event> { pub fn strings_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Option>> { @@ -150,7 +150,7 @@ impl<'event> File<'event> { pub fn integers( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option, value::Error>> { self.integers_filter(section_name, subsection_name, key, &mut |_| true) @@ -161,7 +161,7 @@ impl<'event> File<'event> { pub fn integers_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Option, value::Error>> { diff --git a/git-config/src/file/access/mutate.rs b/git-config/src/file/access/mutate.rs index 03f5ae3c4df..e70c4397436 100644 --- a/git-config/src/file/access/mutate.rs +++ b/git-config/src/file/access/mutate.rs @@ -1,3 +1,4 @@ +use bstr::BStr; use std::borrow::Cow; use git_features::threading::OwnShared; @@ -15,7 +16,7 @@ impl<'event> File<'event> { pub fn section_mut<'a>( &'a mut self, name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, ) -> Result, lookup::existing::Error> { let id = self .section_ids_by_name_and_subname(name.as_ref(), subsection_name)? @@ -42,7 +43,7 @@ impl<'event> File<'event> { pub fn section_mut_or_create_new<'a>( &'a mut self, name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, ) -> Result, section::header::Error> { self.section_mut_or_create_new_filter(name, subsection_name, &mut |_| true) } @@ -52,7 +53,7 @@ impl<'event> File<'event> { pub fn section_mut_or_create_new_filter<'a>( &'a mut self, name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, filter: &mut MetadataFilter, ) -> Result, section::header::Error> { let name = name.as_ref(); @@ -84,7 +85,7 @@ impl<'event> File<'event> { pub fn section_mut_filter<'a>( &'a mut self, name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, filter: &mut MetadataFilter, ) -> Result>, lookup::existing::Error> { let id = self @@ -107,10 +108,11 @@ impl<'event> File<'event> { /// Creating a new empty section: /// /// ``` + /// # use std::borrow::Cow; /// # use git_config::File; /// # use std::convert::TryFrom; /// let mut git_config = git_config::File::default(); - /// let section = git_config.new_section("hello", Some("world".into()))?; + /// let section = git_config.new_section("hello", Some(Cow::Borrowed("world".into())))?; /// let nl = section.newline().to_owned(); /// assert_eq!(git_config.to_string(), format!("[hello \"world\"]{nl}")); /// # Ok::<(), Box>(()) @@ -120,11 +122,12 @@ impl<'event> File<'event> { /// /// ``` /// # use git_config::File; + /// # use std::borrow::Cow; /// # use std::convert::TryFrom; /// # use bstr::ByteSlice; /// # use git_config::parse::section; /// let mut git_config = git_config::File::default(); - /// let mut section = git_config.new_section("hello", Some("world".into()))?; + /// let mut section = git_config.new_section("hello", Some(Cow::Borrowed("world".into())))?; /// section.push(section::Key::try_from("a")?, Some("b".into())); /// let nl = section.newline().to_owned(); /// assert_eq!(git_config.to_string(), format!("[hello \"world\"]{nl}\ta = b{nl}")); @@ -135,7 +138,7 @@ impl<'event> File<'event> { pub fn new_section( &mut self, name: impl Into>, - subsection: impl Into>>, + subsection: impl Into>>, ) -> Result, section::header::Error> { let id = self.push_section_internal(file::Section::new(name, subsection, OwnShared::clone(&self.meta))?); let nl = self.detect_newline_style_smallvec(); @@ -184,7 +187,7 @@ impl<'event> File<'event> { pub fn remove_section<'a>( &mut self, name: &str, - subsection_name: impl Into>, + subsection_name: impl Into>, ) -> Option> { let id = self .section_ids_by_name_and_subname(name, subsection_name.into()) @@ -233,7 +236,7 @@ impl<'event> File<'event> { pub fn remove_section_filter<'a>( &mut self, name: &str, - subsection_name: impl Into>, + subsection_name: impl Into>, filter: &mut MetadataFilter, ) -> Option> { let id = self @@ -268,9 +271,9 @@ impl<'event> File<'event> { pub fn rename_section<'a>( &mut self, name: impl AsRef, - subsection_name: impl Into>, + subsection_name: impl Into>, new_name: impl Into>, - new_subsection_name: impl Into>>, + new_subsection_name: impl Into>>, ) -> Result<(), rename_section::Error> { let id = self .section_ids_by_name_and_subname(name.as_ref(), subsection_name.into())? @@ -290,9 +293,9 @@ impl<'event> File<'event> { pub fn rename_section_filter<'a>( &mut self, name: impl AsRef, - subsection_name: impl Into>, + subsection_name: impl Into>, new_name: impl Into>, - new_subsection_name: impl Into>>, + new_subsection_name: impl Into>>, filter: &mut MetadataFilter, ) -> Result<(), rename_section::Error> { let id = self diff --git a/git-config/src/file/access/raw.rs b/git-config/src/file/access/raw.rs index 2b4fc6da944..c5ac8d4886c 100644 --- a/git-config/src/file/access/raw.rs +++ b/git-config/src/file/access/raw.rs @@ -22,7 +22,7 @@ impl<'event> File<'event> { pub fn raw_value( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Result, lookup::existing::Error> { self.raw_value_filter(section_name, subsection_name, key, &mut |_| true) @@ -36,7 +36,7 @@ impl<'event> File<'event> { pub fn raw_value_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Result, lookup::existing::Error> { @@ -63,7 +63,7 @@ impl<'event> File<'event> { pub fn raw_value_mut<'lookup>( &mut self, section_name: impl AsRef, - subsection_name: Option<&'lookup str>, + subsection_name: Option<&'lookup BStr>, key: &'lookup str, ) -> Result, lookup::existing::Error> { self.raw_value_mut_filter(section_name, subsection_name, key, &mut |_| true) @@ -77,7 +77,7 @@ impl<'event> File<'event> { pub fn raw_value_mut_filter<'lookup>( &mut self, section_name: impl AsRef, - subsection_name: Option<&'lookup str>, + subsection_name: Option<&'lookup BStr>, key: &'lookup str, filter: &mut MetadataFilter, ) -> Result, lookup::existing::Error> { @@ -173,7 +173,7 @@ impl<'event> File<'event> { pub fn raw_values( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Result>, lookup::existing::Error> { self.raw_values_filter(section_name, subsection_name, key, &mut |_| true) @@ -187,7 +187,7 @@ impl<'event> File<'event> { pub fn raw_values_filter( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, filter: &mut MetadataFilter, ) -> Result>, lookup::existing::Error> { @@ -262,7 +262,7 @@ impl<'event> File<'event> { pub fn raw_values_mut<'lookup>( &mut self, section_name: impl AsRef, - subsection_name: Option<&'lookup str>, + subsection_name: Option<&'lookup BStr>, key: &'lookup str, ) -> Result, lookup::existing::Error> { self.raw_values_mut_filter(section_name, subsection_name, key, &mut |_| true) @@ -273,7 +273,7 @@ impl<'event> File<'event> { pub fn raw_values_mut_filter<'lookup>( &mut self, section_name: impl AsRef, - subsection_name: Option<&'lookup str>, + subsection_name: Option<&'lookup BStr>, key: &'lookup str, filter: &mut MetadataFilter, ) -> Result, lookup::existing::Error> { @@ -368,7 +368,7 @@ impl<'event> File<'event> { pub fn set_existing_raw_value<'b>( &mut self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, new_value: impl Into<&'b BStr>, ) -> Result<(), lookup::existing::Error> { @@ -406,7 +406,7 @@ impl<'event> File<'event> { pub fn set_raw_value<'b, Key, E>( &mut self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: Key, new_value: impl Into<&'b BStr>, ) -> Result>, crate::file::set_raw_value::Error> @@ -422,7 +422,7 @@ impl<'event> File<'event> { pub fn set_raw_value_filter<'b, Key, E>( &mut self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: Key, new_value: impl Into<&'b BStr>, filter: &mut MetadataFilter, @@ -522,7 +522,7 @@ impl<'event> File<'event> { pub fn set_existing_raw_multi_value<'a, Iter, Item>( &mut self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, new_values: Iter, ) -> Result<(), lookup::existing::Error> diff --git a/git-config/src/file/access/read_only.rs b/git-config/src/file/access/read_only.rs index ec78aea77de..103626e14d4 100644 --- a/git-config/src/file/access/read_only.rs +++ b/git-config/src/file/access/read_only.rs @@ -52,7 +52,7 @@ impl<'event> File<'event> { pub fn value<'a, T: TryFrom>>( &'a self, section_name: &str, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: &str, ) -> Result> { T::try_from(self.raw_value(section_name, subsection_name, key)?).map_err(lookup::Error::FailedConversion) @@ -62,7 +62,7 @@ impl<'event> File<'event> { pub fn try_value<'a, T: TryFrom>>( &'a self, section_name: &str, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: &str, ) -> Option> { self.raw_value(section_name, subsection_name, key).ok().map(T::try_from) @@ -118,7 +118,7 @@ impl<'event> File<'event> { pub fn values<'a, T: TryFrom>>( &'a self, section_name: &str, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: &str, ) -> Result, lookup::Error> { self.raw_values(section_name, subsection_name, key)? @@ -132,7 +132,7 @@ impl<'event> File<'event> { pub fn section( &self, name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, ) -> Result<&file::Section<'event>, lookup::existing::Error> { Ok(self .section_filter(name, subsection_name, &mut |_| true)? @@ -146,7 +146,7 @@ impl<'event> File<'event> { pub fn section_filter<'a>( &'a self, name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, filter: &mut MetadataFilter, ) -> Result>, lookup::existing::Error> { Ok(self diff --git a/git-config/src/file/init/from_env.rs b/git-config/src/file/init/from_env.rs index aee3c47787a..25fd28ac6b3 100644 --- a/git-config/src/file/init/from_env.rs +++ b/git-config/src/file/init/from_env.rs @@ -1,3 +1,4 @@ +use bstr::BStr; use std::convert::TryFrom; use crate::{file, file::init, parse, parse::section, path::interpolate, File}; @@ -6,6 +7,8 @@ use crate::{file, file::init, parse, parse::section, path::interpolate, File}; #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { + #[error("Configuration {kind} at index {index} contained illformed UTF-8")] + IllformedUtf8 { index: usize, kind: &'static str }, #[error("GIT_CONFIG_COUNT was not a positive integer: {}", .input)] InvalidConfigCount { input: String }, #[error("GIT_CONFIG_KEY_{} was not set", .key_id)] @@ -51,9 +54,12 @@ impl File<'static> { }; let mut config = File::new(meta); for i in 0..count { - let key = env::var(format!("GIT_CONFIG_KEY_{}", i)).map_err(|_| Error::InvalidKeyId { key_id: i })?; + let key = git_path::os_string_into_bstring( + env::var_os(format!("GIT_CONFIG_KEY_{}", i)).ok_or(Error::InvalidKeyId { key_id: i })?, + ) + .map_err(|_| Error::IllformedUtf8 { index: i, kind: "key" })?; let value = env::var_os(format!("GIT_CONFIG_VALUE_{}", i)).ok_or(Error::InvalidValueId { value_id: i })?; - let key = parse::key(&key).ok_or_else(|| Error::InvalidKeyValue { + let key = parse::key(<_ as AsRef>::as_ref(&key)).ok_or_else(|| Error::InvalidKeyValue { key_id: i, key_val: key.to_string(), })?; @@ -64,7 +70,10 @@ impl File<'static> { section::Key::try_from(key.value_name.to_owned())?, Some( git_path::os_str_into_bstr(&value) - .expect("no illformed UTF-8") + .map_err(|_| Error::IllformedUtf8 { + index: i, + kind: "value", + })? .as_ref() .into(), ), diff --git a/git-config/src/file/section/mod.rs b/git-config/src/file/section/mod.rs index 10a5233c51a..392686b7bc4 100644 --- a/git-config/src/file/section/mod.rs +++ b/git-config/src/file/section/mod.rs @@ -1,6 +1,6 @@ use std::{borrow::Cow, ops::Deref}; -use bstr::{BString, ByteSlice}; +use bstr::{BStr, BString, ByteSlice}; use smallvec::SmallVec; use crate::{ @@ -29,7 +29,7 @@ impl<'a> Section<'a> { /// Create a new section with the given `name` and optional, `subsection`, `meta`-data and an empty body. pub fn new( name: impl Into>, - subsection: impl Into>>, + subsection: impl Into>>, meta: impl Into>, ) -> Result { Ok(Section { diff --git a/git-config/src/file/util.rs b/git-config/src/file/util.rs index daa2462a171..6d3de0da41a 100644 --- a/git-config/src/file/util.rs +++ b/git-config/src/file/util.rs @@ -115,7 +115,7 @@ impl<'event> File<'event> { pub(crate) fn section_ids_by_name_and_subname<'a>( &'a self, section_name: &'a str, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, ) -> Result + ExactSizeIterator + DoubleEndedIterator + '_, lookup::existing::Error> { let section_name = section::Name::from_str_unchecked(section_name); diff --git a/git-config/src/parse/key.rs b/git-config/src/parse/key.rs index d4b11bb4707..b0e0376be2a 100644 --- a/git-config/src/parse/key.rs +++ b/git-config/src/parse/key.rs @@ -1,10 +1,12 @@ +use bstr::{BStr, ByteSlice}; + /// An unvalidated parse result of parsing input like `remote.origin.url` or `core.bare`. #[derive(Debug, PartialEq, Ord, PartialOrd, Eq, Hash, Clone, Copy)] pub struct Key<'a> { /// The name of the section, like `core` in `core.bare`. pub section_name: &'a str, /// The name of the sub-section, like `origin` in `remote.origin.url`. - pub subsection_name: Option<&'a str>, + pub subsection_name: Option<&'a BStr>, /// The name of the section key, like `url` in `remote.origin.url`. pub value_name: &'a str, } @@ -12,16 +14,22 @@ pub struct Key<'a> { /// Parse `input` like `core.bare` or `remote.origin.url` as a `Key` to make its fields available, /// or `None` if there were not at least 2 tokens separated by `.`. /// Note that `input` isn't validated, and is `str` as ascii is a subset of UTF-8 which is required for any valid keys. -pub fn parse_unvalidated(input: &str) -> Option> { - let (section_name, subsection_or_key) = input.split_once('.')?; - let (subsection_name, value_name) = match subsection_or_key.rsplit_once('.') { - Some((subsection, key)) => (Some(subsection), key), - None => (None, subsection_or_key), +pub fn parse_unvalidated<'a>(input: impl Into<&'a BStr>) -> Option> { + let input = input.into(); + let mut tokens = input.splitn(2, |b| *b == b'.'); + let section_name = tokens.next()?; + let subsection_or_key = tokens.next()?; + let mut tokens = subsection_or_key.rsplitn(2, |b| *b == b'.'); + let (subsection_name, value_name) = match (tokens.next(), tokens.next()) { + (Some(key), Some(subsection)) => (Some(subsection.into()), key), + (Some(key), None) => (None, key), + (None, Some(_)) => unreachable!("iterator can't restart producing items"), + (None, None) => return None, }; Some(Key { - section_name, + section_name: section_name.to_str().ok()?, subsection_name, - value_name, + value_name: value_name.to_str().ok()?, }) } diff --git a/git-config/src/parse/nom/tests.rs b/git-config/src/parse/nom/tests.rs index e8d6a6a2fbb..f8754ad232c 100644 --- a/git-config/src/parse/nom/tests.rs +++ b/git-config/src/parse/nom/tests.rs @@ -847,6 +847,24 @@ mod key_value_pair { super::key_value_pair(i, node, &mut |e| events.push(e)).map(|t| (t.0, ())) } + #[test] + fn nonascii_is_allowed_for_values_but_not_for_keys() { + let mut node = ParseNode::SectionHeader; + let mut vec = Default::default(); + assert!(key_value("你好".as_bytes(), &mut node, &mut vec).is_err()); + assert!(key_value("a = 你好 ".as_bytes(), &mut node, &mut vec).is_ok()); + assert_eq!( + vec, + into_events(vec![ + name_event("a"), + whitespace_event(" "), + Event::KeyValueSeparator, + whitespace_event(" "), + value_event("你好") + ]) + ); + } + #[test] fn whitespace_is_not_ambigious() { let mut node = ParseNode::SectionHeader; diff --git a/git-config/src/parse/section/header.rs b/git-config/src/parse/section/header.rs index c4c13a97365..9e70b76b3d0 100644 --- a/git-config/src/parse/section/header.rs +++ b/git-config/src/parse/section/header.rs @@ -22,14 +22,14 @@ impl<'a> Header<'a> { /// or `[remote "origin"]` for `subsection` being "origin" and `name` being "remote". pub fn new( name: impl Into>, - subsection: impl Into>>, + subsection: impl Into>>, ) -> Result, Error> { let name = Name(validated_name(into_cow_bstr(name.into()))?); if let Some(subsection_name) = subsection.into() { Ok(Header { name, separator: Some(Cow::Borrowed(" ".into())), - subsection_name: Some(validated_subsection(into_cow_bstr(subsection_name))?), + subsection_name: Some(validated_subsection(subsection_name)?), }) } else { Ok(Header { @@ -71,7 +71,7 @@ mod tests { #[test] fn empty_header_sub_names_are_legal() { assert!( - Header::new("remote", Some("".into())).is_ok(), + Header::new("remote", Some(Cow::Borrowed("".into()))).is_ok(), "yes, git allows this, so do we" ); } diff --git a/git-config/tests/file/access/mutate.rs b/git-config/tests/file/access/mutate.rs index 6fb90ee693d..fe17140412b 100644 --- a/git-config/tests/file/access/mutate.rs +++ b/git-config/tests/file/access/mutate.rs @@ -36,13 +36,13 @@ mod remove_section { assert_eq!(removed.header().subsection_name(), None); assert_eq!(file.sections().count(), 1); - let removed = file.remove_section("core", Some("name")).expect("found"); + let removed = file.remove_section("core", Some("name".into())).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")) + file.section_mut_or_create_new("core", Some("name".into())) .expect("creation succeeds"); } } @@ -60,7 +60,7 @@ mod rename_section { )); assert!(matches!( - file.rename_section("core", None, "new-core", Cow::from("a\nb")), + file.rename_section("core", None, "new-core", Some(Cow::Borrowed("a\nb".into()))), Err(rename_section::Error::Section( section::header::Error::InvalidSubSection )) diff --git a/git-config/tests/file/access/raw/raw_multi_value.rs b/git-config/tests/file/access/raw/raw_multi_value.rs index 7357d52150b..7679ba7104c 100644 --- a/git-config/tests/file/access/raw/raw_multi_value.rs +++ b/git-config/tests/file/access/raw/raw_multi_value.rs @@ -45,7 +45,7 @@ fn section_not_found() -> crate::Result { fn subsection_not_found() -> crate::Result { let config = File::try_from("[core]\na=b\nc=d")?; assert!(matches!( - config.raw_values("core", Some("a"), "a"), + config.raw_values("core", Some("a".into()), "a"), Err(lookup::existing::Error::SubSectionMissing) )); Ok(()) @@ -65,7 +65,7 @@ fn key_not_found() -> crate::Result { fn subsection_must_be_respected() -> crate::Result { let config = File::try_from("[core]a=b\n[core.a]a=c")?; assert_eq!(config.raw_values("core", None, "a")?, vec![cow_str("b")]); - assert_eq!(config.raw_values("core", Some("a"), "a")?, vec![cow_str("c")]); + assert_eq!(config.raw_values("core", Some("a".into()), "a")?, vec![cow_str("c")]); Ok(()) } diff --git a/git-config/tests/file/access/raw/raw_value.rs b/git-config/tests/file/access/raw/raw_value.rs index 70c5c1c7b76..c822f7d396d 100644 --- a/git-config/tests/file/access/raw/raw_value.rs +++ b/git-config/tests/file/access/raw/raw_value.rs @@ -38,7 +38,7 @@ fn section_not_found() -> crate::Result { fn subsection_not_found() -> crate::Result { let config = File::try_from("[core]\na=b\nc=d")?; assert!(matches!( - config.raw_value("core", Some("a"), "a"), + config.raw_value("core", Some("a".into()), "a"), Err(lookup::existing::Error::SubSectionMissing) )); Ok(()) @@ -58,6 +58,6 @@ fn key_not_found() -> crate::Result { fn subsection_must_be_respected() -> crate::Result { let config = File::try_from("[core]a=b\n[core.a]a=c")?; assert_eq!(config.raw_value("core", None, "a")?.as_ref(), "b"); - assert_eq!(config.raw_value("core", Some("a"), "a")?.as_ref(), "c"); + assert_eq!(config.raw_value("core", Some("a".into()), "a")?.as_ref(), "c"); Ok(()) } diff --git a/git-config/tests/file/access/raw/set_raw_value.rs b/git-config/tests/file/access/raw/set_raw_value.rs index 6691a230f95..18eed734730 100644 --- a/git-config/tests/file/access/raw/set_raw_value.rs +++ b/git-config/tests/file/access/raw/set_raw_value.rs @@ -54,11 +54,13 @@ fn comment_included() { fn non_existing_values_cannot_be_set() -> crate::Result { let mut file = git_config::File::default(); file.set_raw_value("new", None, "key", "value")?; - file.set_raw_value("new", "subsection".into(), "key", "subsection-value")?; + file.set_raw_value("new", Some("subsection".into()), "key", "subsection-value")?; assert_eq!(file.string("new", None, "key").expect("present").as_ref(), "value"); assert_eq!( - file.string("new", Some("subsection"), "key").expect("present").as_ref(), + file.string("new", Some("subsection".into()), "key") + .expect("present") + .as_ref(), "subsection-value" ); Ok(()) diff --git a/git-config/tests/file/access/read_only.rs b/git-config/tests/file/access/read_only.rs index 8af11d9adae..4f7acb310b6 100644 --- a/git-config/tests/file/access/read_only.rs +++ b/git-config/tests/file/access/read_only.rs @@ -244,7 +244,7 @@ fn sections_by_name() { "#; let config = File::try_from(config).unwrap(); - let value = config.string("remote", Some("origin"), "url").unwrap(); + let value = config.string("remote", Some("origin".into()), "url").unwrap(); assert_eq!(value, cow_str("git@github.com:Byron/gitoxide.git")); } diff --git a/git-config/tests/file/mutable/section.rs b/git-config/tests/file/mutable/section.rs index 88ff78bde95..b2d94ac68b7 100644 --- a/git-config/tests/file/mutable/section.rs +++ b/git-config/tests/file/mutable/section.rs @@ -7,7 +7,7 @@ fn section_mut_must_exist_as_section_is_not_created_automatically() { #[test] fn section_mut_or_create_new_is_infallible() -> crate::Result { let mut config = multi_value_section(); - let section = config.section_mut_or_create_new("name", Some("subsection"))?; + let section = config.section_mut_or_create_new("name", Some("subsection".into()))?; assert_eq!(section.header().name(), "name"); assert_eq!(section.header().subsection_name().expect("set"), "subsection"); Ok(()) @@ -130,7 +130,7 @@ mod push { #[test] fn none_as_value_omits_the_key_value_separator() -> crate::Result { let mut file = git_config::File::default(); - let mut section = file.section_mut_or_create_new("a", Some("sub"))?; + let mut section = file.section_mut_or_create_new("a", Some("sub".into()))?; section.push("key".try_into()?, None); let expected = format!("[a \"sub\"]{nl}\tkey{nl}", nl = section.newline()); assert_eq!(section.value("key"), None, "single value counts as None"); diff --git a/git-config/tests/parse/key.rs b/git-config/tests/parse/key.rs index af46e647486..d0d3a427a5b 100644 --- a/git-config/tests/parse/key.rs +++ b/git-config/tests/parse/key.rs @@ -23,7 +23,7 @@ fn section_name_subsection_and_key() { parse::key("remote.origin.url"), Some(parse::Key { section_name: "remote", - subsection_name: Some("origin"), + subsection_name: Some("origin".into()), value_name: "url" }) ); @@ -32,7 +32,7 @@ fn section_name_subsection_and_key() { parse::key("includeIf.gitdir/i:C:\\bare.git.path"), Some(parse::Key { section_name: "includeIf", - subsection_name: Some("gitdir/i:C:\\bare.git"), + subsection_name: Some("gitdir/i:C:\\bare.git".into()), value_name: "path" }) ); diff --git a/git-config/tests/parse/section.rs b/git-config/tests/parse/section.rs index 35333a7e601..aae28a98178 100644 --- a/git-config/tests/parse/section.rs +++ b/git-config/tests/parse/section.rs @@ -3,23 +3,28 @@ use std::borrow::Cow; use git_config::parse::{section, Event}; pub fn header_event(name: &'static str, subsection: impl Into>) -> Event<'static> { - Event::SectionHeader(section::Header::new(name, subsection.into().map(Cow::Borrowed)).unwrap()) + Event::SectionHeader(section::Header::new(name, subsection.into().map(|s| Cow::Borrowed(s.into()))).unwrap()) } mod header { - mod write_to { - use std::borrow::Cow; + use bstr::BStr; + use std::borrow::Cow; + fn cow_section(name: &str) -> Option> { + Some(Cow::Borrowed(name.into())) + } + mod write_to { + use crate::parse::section::header::cow_section; use git_config::parse::section; #[test] fn subsection_backslashes_and_quotes_are_escaped() -> crate::Result { assert_eq!( - section::Header::new("core", Cow::from(r#"a\b"#))?.to_bstring(), + section::Header::new("core", cow_section(r#"a\b"#))?.to_bstring(), r#"[core "a\\b"]"# ); assert_eq!( - section::Header::new("core", Cow::from(r#"a:"b""#))?.to_bstring(), + section::Header::new("core", cow_section(r#"a:"b""#))?.to_bstring(), r#"[core "a:\"b\""]"# ); Ok(()) @@ -28,15 +33,14 @@ mod header { #[test] fn everything_is_allowed() -> crate::Result { assert_eq!( - section::Header::new("core", Cow::from("a/b \t\t a\\b"))?.to_bstring(), + section::Header::new("core", cow_section("a/b \t\t a\\b"))?.to_bstring(), "[core \"a/b \t\t a\\\\b\"]" ); Ok(()) } } mod new { - use std::borrow::Cow; - + use crate::parse::section::header::cow_section; use git_config::parse::section; #[test] @@ -52,11 +56,11 @@ mod header { #[test] fn subsections_with_newlines_and_null_bytes_are_rejected() { assert_eq!( - section::Header::new("a", Cow::from("a\nb")), + section::Header::new("a", cow_section("a\nb")), Err(section::header::Error::InvalidSubSection) ); assert_eq!( - section::Header::new("a", Cow::from("a\0b")), + section::Header::new("a", cow_section("a\0b")), Err(section::header::Error::InvalidSubSection) ); } From 5fa95460db843f7dcfe68002b303b8b7649846dd Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 13:41:18 +0100 Subject: [PATCH 03/29] feat: comfort API like `string_by_key(key)` takes a key like `"remote.origin.url"`, add `section_by_key("remote.origin")` as well. That way it's the most comfortable way to query values and very similar to how git does it, too. Additionally, sections can be obtained by section key, both mutably and immutably for completeness. --- git-config/src/file/access/comfort.rs | 92 +++++++++++++++++++ git-config/src/file/access/mutate.rs | 20 ++++ git-config/src/file/access/read_only.rs | 20 ++++ git-config/src/parse/section/mod.rs | 2 + git-config/src/parse/section/unvalidated.rs | 25 +++++ git-config/src/parse/tests.rs | 37 ++++++++ git-config/tests/file/access/read_only.rs | 7 ++ git-config/tests/file/init/comfort.rs | 1 + git-config/tests/file/init/from_env.rs | 2 +- .../init/from_paths/includes/unconditional.rs | 2 + git-config/tests/file/init/from_paths/mod.rs | 8 ++ git-config/tests/file/mutable/section.rs | 2 +- 12 files changed, 216 insertions(+), 2 deletions(-) create mode 100644 git-config/src/parse/section/unvalidated.rs diff --git a/git-config/src/file/access/comfort.rs b/git-config/src/file/access/comfort.rs index c81557fbb44..b4953c59715 100644 --- a/git-config/src/file/access/comfort.rs +++ b/git-config/src/file/access/comfort.rs @@ -18,6 +18,11 @@ impl<'event> File<'event> { self.string_filter(section_name, subsection_name, key, &mut |_| true) } + /// Like [`string()`][File::string()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn string_by_key<'a>(&self, key: impl Into<&'a BStr>) -> Option> { + self.string_filter_by_key(key, &mut |_| true) + } + /// Like [`string()`][File::string()], but the section containing the returned value must pass `filter` as well. pub fn string_filter( &self, @@ -29,6 +34,17 @@ impl<'event> File<'event> { self.raw_value_filter(section_name, subsection_name, key, filter).ok() } + /// Like [`string_filter()`][File::string_filter()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn string_filter_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + filter: &mut MetadataFilter, + ) -> Option> { + let key = crate::parse::key(key)?; + self.raw_value_filter(key.section_name, key.subsection_name, key.value_name, filter) + .ok() + } + /// Like [`value()`][File::value()], but returning `None` if the path wasn't found. /// /// Note that this path is not vetted and should only point to resources which can't be used @@ -44,6 +60,11 @@ impl<'event> File<'event> { self.path_filter(section_name, subsection_name, key, &mut |_| true) } + /// Like [`path()`][File::path()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn path_by_key<'a>(&self, key: impl Into<&'a BStr>) -> Option> { + self.path_filter_by_key(key, &mut |_| true) + } + /// Like [`path()`][File::path()], but the section containing the returned value must pass `filter` as well. /// /// This should be the preferred way of accessing paths as those from untrusted @@ -62,6 +83,16 @@ impl<'event> File<'event> { .map(crate::Path::from) } + /// Like [`path_filter()`][File::path_filter()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn path_filter_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + filter: &mut MetadataFilter, + ) -> Option> { + let key = crate::parse::key(key)?; + self.path_filter(key.section_name, key.subsection_name, key.value_name, filter) + } + /// Like [`value()`][File::value()], but returning `None` if the boolean value wasn't found. pub fn boolean( &self, @@ -72,6 +103,11 @@ impl<'event> File<'event> { self.boolean_filter(section_name, subsection_name, key, &mut |_| true) } + /// Like [`boolean()`][File::boolean()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn boolean_by_key<'a>(&self, key: impl Into<&'a BStr>) -> Option> { + self.boolean_filter_by_key(key, &mut |_| true) + } + /// Like [`boolean()`][File::boolean()], but the section containing the returned value must pass `filter` as well. pub fn boolean_filter( &self, @@ -99,6 +135,16 @@ impl<'event> File<'event> { None } + /// Like [`boolean_filter()`][File::boolean_filter()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn boolean_filter_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + filter: &mut MetadataFilter, + ) -> Option> { + let key = crate::parse::key(key)?; + self.boolean_filter(key.section_name, key.subsection_name, key.value_name, filter) + } + /// Like [`value()`][File::value()], but returning an `Option` if the integer wasn't found. pub fn integer( &self, @@ -109,6 +155,11 @@ impl<'event> File<'event> { self.integer_filter(section_name, subsection_name, key, &mut |_| true) } + /// Like [`integer()`][File::integer()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn integer_by_key<'a>(&self, key: impl Into<&'a BStr>) -> Option> { + self.integer_filter_by_key(key, &mut |_| true) + } + /// Like [`integer()`][File::integer()], but the section containing the returned value must pass `filter` as well. pub fn integer_filter( &self, @@ -124,6 +175,16 @@ impl<'event> File<'event> { })) } + /// Like [`integer_filter()`][File::integer_filter()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn integer_filter_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + filter: &mut MetadataFilter, + ) -> Option> { + let key = crate::parse::key(key)?; + self.integer_filter(key.section_name, key.subsection_name, key.value_name, filter) + } + /// Similar to [`values(…)`][File::values()] but returning strings if at least one of them was found. pub fn strings( &self, @@ -134,6 +195,12 @@ impl<'event> File<'event> { self.raw_values(section_name, subsection_name, key).ok() } + /// Like [`strings()`][File::strings()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn strings_by_key<'a>(&self, key: impl Into<&'a BStr>) -> Option>> { + let key = crate::parse::key(key)?; + self.strings(key.section_name, key.subsection_name, key.value_name) + } + /// Similar to [`strings(…)`][File::strings()], but all values are in sections that passed `filter`. pub fn strings_filter( &self, @@ -145,6 +212,16 @@ impl<'event> File<'event> { self.raw_values_filter(section_name, subsection_name, key, filter).ok() } + /// Like [`strings_filter()`][File::strings_filter()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn strings_filter_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + filter: &mut MetadataFilter, + ) -> Option>> { + let key = crate::parse::key(key)?; + self.strings_filter(key.section_name, key.subsection_name, key.value_name, filter) + } + /// Similar to [`values(…)`][File::values()] but returning integers if at least one of them was found /// and if none of them overflows. pub fn integers( @@ -156,6 +233,11 @@ impl<'event> File<'event> { self.integers_filter(section_name, subsection_name, key, &mut |_| true) } + /// Like [`integers()`][File::integers()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn integers_by_key<'a>(&self, key: impl Into<&'a BStr>) -> Option, value::Error>> { + self.integers_filter_by_key(key, &mut |_| true) + } + /// Similar to [`integers(…)`][File::integers()] but all integers are in sections that passed `filter` /// and that are not overflowing. pub fn integers_filter( @@ -179,4 +261,14 @@ impl<'event> File<'event> { .collect() }) } + + /// Like [`integers_filter()`][File::integers_filter()], but suitable for statically known `key`s like `remote.origin.url`. + pub fn integers_filter_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + filter: &mut MetadataFilter, + ) -> Option, value::Error>> { + let key = crate::parse::key(key)?; + self.integers_filter(key.section_name, key.subsection_name, key.value_name, filter) + } } diff --git a/git-config/src/file/access/mutate.rs b/git-config/src/file/access/mutate.rs index e70c4397436..3a424b1389b 100644 --- a/git-config/src/file/access/mutate.rs +++ b/git-config/src/file/access/mutate.rs @@ -31,6 +31,15 @@ impl<'event> File<'event> { .to_mut(nl)) } + /// Returns the last found mutable section with a given `key`, identifying the name and subsection name like `core` or `remote.origin`. + pub fn section_mut_by_key<'a, 'b>( + &'a mut self, + key: impl Into<&'b BStr>, + ) -> Result, lookup::existing::Error> { + let key = section::unvalidated::Key::parse(key).ok_or(lookup::existing::Error::KeyMissing)?; + self.section_mut(key.section_name, key.subsection_name) + } + /// Return the mutable section identified by `id`, or `None` if it didn't exist. /// /// Note that `id` is stable across deletions and insertions. @@ -99,6 +108,17 @@ impl<'event> File<'event> { Ok(id.and_then(move |id| self.sections.get_mut(&id).map(move |s| s.to_mut(nl)))) } + /// Like [`section_mut_filter()`][File::section_mut_filter()], but identifies the with a given `key`, + /// like `core` or `remote.origin`. + pub fn section_mut_filter_by_key<'a, 'b>( + &'a mut self, + key: impl Into<&'b BStr>, + filter: &mut MetadataFilter, + ) -> Result>, lookup::existing::Error> { + let key = section::unvalidated::Key::parse(key).ok_or(lookup::existing::Error::KeyMissing)?; + self.section_mut_filter(key.section_name, key.subsection_name, filter) + } + /// Adds a new section. If a subsection name was provided, then /// the generated header will use the modern subsection syntax. /// Returns a reference to the new section for immediate editing. diff --git a/git-config/src/file/access/read_only.rs b/git-config/src/file/access/read_only.rs index 103626e14d4..5352e1982be 100644 --- a/git-config/src/file/access/read_only.rs +++ b/git-config/src/file/access/read_only.rs @@ -139,6 +139,16 @@ impl<'event> File<'event> { .expect("section present as we take all")) } + /// Returns the last found immutable section with a given `key`, identifying the name and subsection name like `core` + /// or `remote.origin`. + pub fn section_by_key<'a>( + &self, + key: impl Into<&'a BStr>, + ) -> Result<&file::Section<'event>, lookup::existing::Error> { + let key = crate::parse::section::unvalidated::Key::parse(key).ok_or(lookup::existing::Error::KeyMissing)?; + self.section(key.section_name, key.subsection_name) + } + /// Returns the last found immutable section with a given `name` and optional `subsection_name`, that matches `filter`. /// /// If there are sections matching `section_name` and `subsection_name` but the `filter` rejects all of them, `Ok(None)` @@ -161,6 +171,16 @@ impl<'event> File<'event> { })) } + /// Like [`section_filter()`][File::section_filter()], but identifies the section with `key` like `core` or `remote.origin`. + pub fn section_filter_by_key<'a, 'b>( + &'a self, + key: impl Into<&'b BStr>, + filter: &mut MetadataFilter, + ) -> Result>, lookup::existing::Error> { + let key = crate::parse::section::unvalidated::Key::parse(key).ok_or(lookup::existing::Error::KeyMissing)?; + self.section_filter(key.section_name, key.subsection_name, filter) + } + /// Gets all sections that match the provided `name`, ignoring any subsections. /// /// # Examples diff --git a/git-config/src/parse/section/mod.rs b/git-config/src/parse/section/mod.rs index 040efa944b0..7ba08b87d64 100644 --- a/git-config/src/parse/section/mod.rs +++ b/git-config/src/parse/section/mod.rs @@ -8,6 +8,8 @@ use crate::parse::{Event, Section}; /// pub mod header; +pub(crate) mod unvalidated; + /// A container for events, avoiding heap allocations in typical files. pub type Events<'a> = SmallVec<[Event<'a>; 64]>; diff --git a/git-config/src/parse/section/unvalidated.rs b/git-config/src/parse/section/unvalidated.rs new file mode 100644 index 00000000000..1710837fe0f --- /dev/null +++ b/git-config/src/parse/section/unvalidated.rs @@ -0,0 +1,25 @@ +use bstr::{BStr, ByteSlice}; + +/// An unvalidated parse result of a key for a section, parsing input like `remote.origin` or `core`. +#[derive(Debug, PartialEq, Ord, PartialOrd, Eq, Hash, Clone, Copy)] +pub struct Key<'a> { + /// The name of the section, like `remote` in `remote.origin`. + pub section_name: &'a str, + /// The name of the sub-section, like `origin` in `remote.origin`. + pub subsection_name: Option<&'a BStr>, +} + +impl<'a> Key<'a> { + /// Parse `input` like `remote.origin` or `core` as a `Key` to make its section specific fields available, + /// or `None` if there were not one or two tokens separated by `.`. + /// Note that `input` isn't validated, and is `str` as ascii is a subset of UTF-8 which is required for any valid keys. + pub fn parse(input: impl Into<&'a BStr>) -> Option { + let input = input.into(); + let mut tokens = input.splitn(2, |b| *b == b'.'); + + Some(Key { + section_name: tokens.next()?.to_str().ok()?, + subsection_name: tokens.next().map(Into::into), + }) + } +} diff --git a/git-config/src/parse/tests.rs b/git-config/src/parse/tests.rs index 790c1fee5af..2a2853c4c12 100644 --- a/git-config/src/parse/tests.rs +++ b/git-config/src/parse/tests.rs @@ -1,6 +1,43 @@ mod section { mod header { + mod unvalidated { + use crate::parse::section::unvalidated::Key; + + #[test] + fn section_name_only() { + assert_eq!( + Key::parse("core").unwrap(), + Key { + section_name: "core", + subsection_name: None + } + ); + } + + #[test] + fn section_name_and_subsection() { + assert_eq!( + Key::parse("core.bare").unwrap(), + Key { + section_name: "core", + subsection_name: Some("bare".into()) + } + ); + } + + #[test] + fn section_name_and_subsection_with_separators() { + assert_eq!( + Key::parse("remote.https:///home/user.git").unwrap(), + Key { + section_name: "remote", + subsection_name: Some("https:///home/user.git".into()) + } + ); + } + } + mod write_to { use std::borrow::Cow; diff --git a/git-config/tests/file/access/read_only.rs b/git-config/tests/file/access/read_only.rs index 4f7acb310b6..a061cea3fcb 100644 --- a/git-config/tests/file/access/read_only.rs +++ b/git-config/tests/file/access/read_only.rs @@ -39,6 +39,7 @@ fn get_value_for_all_provided_values() -> crate::Result { assert!(!config.value::("core", None, "bool-explicit")?.0); assert!(!config.boolean("core", None, "bool-explicit").expect("exists")?); + assert!(!config.boolean_by_key("core.bool-explicit").expect("exists")?); assert!( config.value::("core", None, "bool-implicit").is_err(), @@ -58,6 +59,10 @@ fn get_value_for_all_provided_values() -> crate::Result { &[cow_str("")], "unset values show up as empty within a string array" ); + assert_eq!( + config.strings_by_key("core.bool-implicit").expect("present"), + &[cow_str("")], + ); assert_eq!(config.string("doesnt", None, "exist"), None); @@ -145,6 +150,8 @@ fn get_value_for_all_provided_values() -> crate::Result { let actual = config.path("core", None, "location").expect("present"); assert_eq!(&*actual, "~/tmp"); + let actual = config.path_by_key("core.location").expect("present"); + assert_eq!(&*actual, "~/tmp"); let actual = config.path("core", None, "location-quoted").expect("present"); assert_eq!(&*actual, "~/quoted"); diff --git a/git-config/tests/file/init/comfort.rs b/git-config/tests/file/init/comfort.rs index 1a759344df0..b07137278e3 100644 --- a/git-config/tests/file/init/comfort.rs +++ b/git-config/tests/file/init/comfort.rs @@ -43,6 +43,7 @@ fn from_git_dir() -> crate::Result { "value", "a value from the local repo configuration" ); + assert_eq!(config.string_by_key("a.local").expect("present").as_ref(), "value",); assert_eq!( config.string("a", None, "local-include").expect("present").as_ref(), "from-a.config", diff --git a/git-config/tests/file/init/from_env.rs b/git-config/tests/file/init/from_env.rs index 5ede003d3f2..ceafe6cf0d5 100644 --- a/git-config/tests/file/init/from_env.rs +++ b/git-config/tests/file/init/from_env.rs @@ -44,7 +44,7 @@ fn single_key_value_pair() -> crate::Result { let config = File::from_env(Default::default())?.unwrap(); assert_eq!(config.raw_value("core", None, "key")?, Cow::<[u8]>::Borrowed(b"value")); assert_eq!( - config.section("core", None)?.meta(), + config.section_by_key("core")?.meta(), &git_config::file::Metadata::from(git_config::Source::Env), "source if configured correctly" ); diff --git a/git-config/tests/file/init/from_paths/includes/unconditional.rs b/git-config/tests/file/init/from_paths/includes/unconditional.rs index 6e586a240e4..c9fcc111a10 100644 --- a/git-config/tests/file/init/from_paths/includes/unconditional.rs +++ b/git-config/tests/file/init/from_paths/includes/unconditional.rs @@ -122,6 +122,7 @@ fn respect_max_depth() -> crate::Result { let config = File::from_paths_metadata(into_meta(vec![dir.path().join("0")]), follow_options())?.expect("non-empty"); assert_eq!(config.integers("core", None, "i"), Some(Ok(vec![0, 1, 2, 3, 4]))); + assert_eq!(config.integers_by_key("core.i"), Some(Ok(vec![0, 1, 2, 3, 4]))); fn make_options(max_depth: u8, error_on_max_depth_exceeded: bool) -> init::Options<'static> { init::Options { @@ -139,6 +140,7 @@ fn respect_max_depth() -> crate::Result { let options = make_options(1, false); let config = File::from_paths_metadata(into_meta(vec![dir.path().join("0")]), options)?.expect("non-empty"); assert_eq!(config.integer("core", None, "i"), Some(Ok(1))); + assert_eq!(config.integer_by_key("core.i"), Some(Ok(1))); // with default max_allowed_depth of 10 and 4 levels of includes, last level is read let options = init::Options { diff --git a/git-config/tests/file/init/from_paths/mod.rs b/git-config/tests/file/init/from_paths/mod.rs index 27aab932388..a5a937072ce 100644 --- a/git-config/tests/file/init/from_paths/mod.rs +++ b/git-config/tests/file/init/from_paths/mod.rs @@ -177,12 +177,20 @@ fn multiple_paths_multi_value_and_filter() -> crate::Result { Some(cow_str("a")), "the filter discards all values with higher priority" ); + assert_eq!( + config.string_filter_by_key("core.key", &mut |m| m.source == Source::System), + Some(cow_str("a")), + ); assert_eq!( config.strings_filter("core", None, "key", &mut |m| m.source == Source::Git || m.source == Source::User), Some(vec![cow_str("b"), cow_str("c")]) ); + assert_eq!( + config.strings_filter_by_key("core.key", &mut |m| m.source == Source::Git || m.source == Source::User), + Some(vec![cow_str("b"), cow_str("c")]) + ); assert_eq!( config.strings("include", None, "path"), diff --git a/git-config/tests/file/mutable/section.rs b/git-config/tests/file/mutable/section.rs index b2d94ac68b7..83e13e69166 100644 --- a/git-config/tests/file/mutable/section.rs +++ b/git-config/tests/file/mutable/section.rs @@ -69,7 +69,7 @@ mod pop { #[test] fn all() -> crate::Result { let mut config = multi_value_section(); - let mut section = config.section_mut("a", None)?; + let mut section = config.section_mut_by_key("a")?; assert_eq!(section.num_values(), 5); assert_eq!(section.keys().count(), 5); From 84d594caf3f04f1ce337e455343278a6f4674957 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 09:28:42 +0100 Subject: [PATCH 04/29] feat!: more type-safety for remote names by making clear they can be named after URLs. `reference::remote::Name` was moved to `remote::Name` to represent all remote names without necessarily having been validated. Note that we also don't validate names anymore if read from configuration, but use it as is. This means that remotes can be read and written back even with invalid names. --- git-repository/src/clone/fetch/mod.rs | 11 ++--- git-repository/src/clone/fetch/util.rs | 7 ++-- git-repository/src/clone/mod.rs | 3 +- git-repository/src/config/cache/access.rs | 3 +- git-repository/src/config/snapshot/access.rs | 14 +++---- .../reference/{remote/mod.rs => remote.rs} | 26 +++--------- git-repository/src/remote/access.rs | 8 +--- git-repository/src/remote/errors.rs | 4 +- git-repository/src/remote/init.rs | 7 ++-- git-repository/src/remote/mod.rs | 40 ++++++++----------- .../src/{reference => }/remote/name.rs | 29 +++++++++++++- git-repository/src/remote/save.rs | 9 +++-- git-repository/src/repository/config/mod.rs | 16 +++++--- git-repository/src/repository/remote.rs | 40 ++++++++++++------- git-repository/src/types.rs | 4 +- git-repository/tests/clone/mod.rs | 8 ++-- git-repository/tests/reference/remote.rs | 6 ++- git-repository/tests/remote/save.rs | 8 ++-- .../repository/config/config_snapshot/mod.rs | 4 +- git-repository/tests/repository/remote.rs | 5 ++- 20 files changed, 140 insertions(+), 112 deletions(-) rename git-repository/src/reference/{remote/mod.rs => remote.rs} (73%) rename git-repository/src/{reference => }/remote/name.rs (62%) diff --git a/git-repository/src/clone/fetch/mod.rs b/git-repository/src/clone/fetch/mod.rs index f5ac611572a..f02b425fda8 100644 --- a/git-repository/src/clone/fetch/mod.rs +++ b/git-repository/src/clone/fetch/mod.rs @@ -1,3 +1,4 @@ +use crate::bstr::BString; use crate::{clone::PrepareFetch, Repository}; /// The error returned by [`PrepareFetch::fetch_only()`]. @@ -58,13 +59,13 @@ impl PrepareFetch { .as_mut() .expect("user error: multiple calls are allowed only until it succeeds"); - let remote_name = match self.remote_name.as_deref() { + let remote_name = match self.remote_name.as_ref() { Some(name) => name.to_owned(), None => repo .config .resolved - .string("clone", None, "defaultRemoteName") - .map(|n| crate::remote::name::validated(n.to_string())) + .string_by_key("clone.defaultRemoteName") + .map(|n| crate::remote::name::validated(n.into_owned())) .unwrap_or_else(|| Ok("origin".into()))?, }; @@ -114,7 +115,7 @@ impl PrepareFetch { repo, &outcome.ref_map.remote_refs, reflog_message.as_ref(), - &remote_name, + remote_name.as_ref(), )?; Ok((self.repo.take().expect("still present"), outcome)) @@ -161,7 +162,7 @@ impl PrepareFetch { /// [`configure_remote()`][Self::configure_remote()]. /// /// If not set here, it defaults to `origin` or the value of `clone.defaultRemoteName`. - pub fn with_remote_name(mut self, name: impl Into) -> Result { + pub fn with_remote_name(mut self, name: impl Into) -> Result { self.remote_name = Some(crate::remote::name::validated(name)?); Ok(self) } diff --git a/git-repository/src/clone/fetch/util.rs b/git-repository/src/clone/fetch/util.rs index 9685c277cf8..6f763041b3c 100644 --- a/git-repository/src/clone/fetch/util.rs +++ b/git-repository/src/clone/fetch/util.rs @@ -7,6 +7,7 @@ use git_ref::{ }; use super::Error; +use crate::bstr::BString; use crate::{ bstr::{BStr, ByteSlice}, Repository, @@ -14,7 +15,7 @@ use crate::{ pub fn write_remote_to_local_config_file( remote: &mut crate::Remote<'_>, - remote_name: String, + remote_name: BString, ) -> Result, Error> { let mut metadata = git_config::file::Metadata::from(git_config::Source::Local); let config_path = remote.repo.git_dir().join("config"); @@ -50,7 +51,7 @@ pub fn update_head( repo: &mut Repository, remote_refs: &[git_protocol::handshake::Ref], reflog_message: &BStr, - remote_name: &str, + remote_name: &BStr, ) -> Result<(), Error> { use git_ref::{ transaction::{PreviousValue, RefEdit}, @@ -171,7 +172,7 @@ fn setup_branch_config( repo: &mut Repository, branch: &FullNameRef, branch_id: Option<&git_hash::oid>, - remote_name: &str, + remote_name: &BStr, ) -> Result<(), Error> { let short_name = match branch.category_and_short_name() { Some((cat, shortened)) if cat == git_ref::Category::LocalBranch => match shortened.to_str() { diff --git a/git-repository/src/clone/mod.rs b/git-repository/src/clone/mod.rs index b3f7b8701e0..1591ea7b6a1 100644 --- a/git-repository/src/clone/mod.rs +++ b/git-repository/src/clone/mod.rs @@ -1,3 +1,4 @@ +use crate::bstr::BString; use std::convert::TryInto; type ConfigureRemoteFn = Box) -> Result, crate::remote::init::Error>>; @@ -9,7 +10,7 @@ pub struct PrepareFetch { /// A freshly initialized repository which is owned by us, or `None` if it was handed to the user repo: Option, /// The name of the remote, which defaults to `origin` if not overridden. - remote_name: Option, + remote_name: Option, /// A function to configure a remote prior to fetching a pack. configure_remote: Option, /// Options for preparing a fetch operation. diff --git a/git-repository/src/config/cache/access.rs b/git-repository/src/config/cache/access.rs index 81fa50165cb..dce0168d958 100644 --- a/git-repository/src/config/cache/access.rs +++ b/git-repository/src/config/cache/access.rs @@ -2,6 +2,7 @@ use std::{borrow::Cow, convert::TryInto, path::PathBuf, time::Duration}; use git_lock::acquire::Fail; +use crate::bstr::BStr; use crate::{ config::{cache::util::ApplyLeniencyDefault, checkout_options, Cache}, remote, @@ -121,7 +122,7 @@ impl Cache { pub(crate) fn trusted_file_path( &self, section_name: impl AsRef, - subsection_name: Option<&str>, + subsection_name: Option<&BStr>, key: impl AsRef, ) -> Option, git_config::path::interpolate::Error>> { let path = self.resolved.path_filter( diff --git a/git-repository/src/config/snapshot/access.rs b/git-repository/src/config/snapshot/access.rs index 3f4e268b6dd..fa66ba77256 100644 --- a/git-repository/src/config/snapshot/access.rs +++ b/git-repository/src/config/snapshot/access.rs @@ -19,12 +19,12 @@ impl<'repo> Snapshot<'repo> { /// For a non-degenerating version, use [`try_boolean(…)`][Self::try_boolean()]. /// /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust. - pub fn boolean(&self, key: &str) -> Option { + pub fn boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option { self.try_boolean(key).and_then(Result::ok) } /// Like [`boolean()`][Self::boolean()], but it will report an error if the value couldn't be interpreted as boolean. - pub fn try_boolean(&self, key: &str) -> Option> { + pub fn try_boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option> { let key = git_config::parse::key(key)?; self.repo .config @@ -38,12 +38,12 @@ impl<'repo> Snapshot<'repo> { /// For a non-degenerating version, use [`try_integer(…)`][Self::try_integer()]. /// /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust. - pub fn integer(&self, key: &str) -> Option { + pub fn integer<'a>(&self, key: impl Into<&'a BStr>) -> Option { self.try_integer(key).and_then(Result::ok) } /// Like [`integer()`][Self::integer()], but it will report an error if the value couldn't be interpreted as boolean. - pub fn try_integer(&self, key: &str) -> Option> { + pub fn try_integer<'a>(&self, key: impl Into<&'a BStr>) -> Option> { let key = git_config::parse::key(key)?; self.repo .config @@ -54,7 +54,7 @@ impl<'repo> Snapshot<'repo> { /// Return the string at `key`, or `None` if there is no such value. /// /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust. - pub fn string(&self, key: &str) -> Option> { + pub fn string<'a>(&self, key: impl Into<&'a BStr>) -> Option> { let key = git_config::parse::key(key)?; self.repo .config @@ -65,9 +65,9 @@ impl<'repo> Snapshot<'repo> { /// Return the trusted and fully interpolated path at `key`, or `None` if there is no such value /// or if no value was found in a trusted file. /// An error occurs if the path could not be interpolated to its final value. - pub fn trusted_path( + pub fn trusted_path<'a>( &self, - key: &str, + key: impl Into<&'a BStr>, ) -> Option, git_config::path::interpolate::Error>> { let key = git_config::parse::key(key)?; self.repo diff --git a/git-repository/src/reference/remote/mod.rs b/git-repository/src/reference/remote.rs similarity index 73% rename from git-repository/src/reference/remote/mod.rs rename to git-repository/src/reference/remote.rs index b0c72c86ae2..83da9f7fc10 100644 --- a/git-repository/src/reference/remote/mod.rs +++ b/git-repository/src/reference/remote.rs @@ -1,20 +1,6 @@ -use std::{borrow::Cow, convert::TryInto}; +use std::convert::TryInto; -use crate::{ - bstr::{BStr, ByteSlice}, - remote, Reference, -}; - -/// The name of a remote, either interpreted as symbol like `origin` or as url as returned by [`Reference::remote_name()`]. -#[derive(Debug, PartialEq, Eq, Clone)] -pub enum Name<'repo> { - /// A symbolic name, like `origin` - Symbol(Cow<'repo, str>), - /// A url pointing to the remote host directly. - Url(Cow<'repo, BStr>), -} - -mod name; +use crate::{remote, Reference}; /// Remotes impl<'repo> Reference<'repo> { @@ -29,8 +15,8 @@ impl<'repo> Reference<'repo> { /// - it's recommended to use the [`remote(…)`][Self::remote()] method as it will configure the remote with additional /// information. /// - `branch..pushRemote` falls back to `branch..remote`. - pub fn remote_name(&self, direction: remote::Direction) -> Option> { - let name = self.name().shorten().to_str().ok()?; + pub fn remote_name(&self, direction: remote::Direction) -> Option> { + let name = self.name().shorten(); let config = &self.repo.config.resolved; (direction == remote::Direction::Push) .then(|| { @@ -54,8 +40,8 @@ impl<'repo> Reference<'repo> { ) -> Option, remote::find::existing::Error>> { // TODO: use `branch..merge` self.remote_name(direction).map(|name| match name { - Name::Symbol(name) => self.repo.find_remote(name.as_ref()).map_err(Into::into), - Name::Url(url) => git_url::parse(url.as_ref()).map_err(Into::into).and_then(|url| { + remote::Name::Symbol(name) => self.repo.find_remote(name.as_ref()).map_err(Into::into), + remote::Name::Url(url) => git_url::parse(url.as_ref()).map_err(Into::into).and_then(|url| { self.repo .remote_at(url) .map_err(|err| remote::find::existing::Error::Find(remote::find::Error::Init(err))) diff --git a/git-repository/src/remote/access.rs b/git-repository/src/remote/access.rs index 534cd505a30..c18f7ea4a84 100644 --- a/git-repository/src/remote/access.rs +++ b/git-repository/src/remote/access.rs @@ -5,12 +5,8 @@ use crate::{bstr::BStr, remote, Remote}; /// Access impl<'repo> Remote<'repo> { /// Return the name of this remote or `None` if it wasn't persisted to disk yet. - // TODO: name can also be a URL but we don't see it like this. There is a problem with accessing such names - // too as they would require a BStr, but valid subsection names are strings, so some degeneration must happen - // for access at least. Argh. Probably use `reference::remote::Name` and turn it into `remote::Name` as it's - // actually correct. - pub fn name(&self) -> Option<&str> { - self.name.as_deref() + pub fn name(&self) -> Option<&remote::Name<'static>> { + self.name.as_ref() } /// Return our repository reference. diff --git a/git-repository/src/remote/errors.rs b/git-repository/src/remote/errors.rs index d6a29963fe5..b73e12d0300 100644 --- a/git-repository/src/remote/errors.rs +++ b/git-repository/src/remote/errors.rs @@ -26,6 +26,8 @@ pub mod find { /// pub mod existing { + use crate::bstr::BString; + /// The error returned by [`Repository::find_remote(…)`][crate::Repository::find_remote()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] @@ -35,7 +37,7 @@ pub mod find { #[error("remote name could not be parsed as URL")] UrlParse(#[from] git_url::parse::Error), #[error("The remote named {name:?} did not exist")] - NotFound { name: String }, + NotFound { name: BString }, } } } diff --git a/git-repository/src/remote/init.rs b/git-repository/src/remote/init.rs index bb29280c7a6..64d5fd97ff7 100644 --- a/git-repository/src/remote/init.rs +++ b/git-repository/src/remote/init.rs @@ -11,8 +11,6 @@ mod error { #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { - #[error(transparent)] - Name(#[from] crate::remote::name::Error), #[error(transparent)] Url(#[from] git_url::parse::Error), #[error("The rewritten {kind} url {rewritten_url:?} failed to parse")] @@ -23,12 +21,13 @@ mod error { }, } } +use crate::bstr::BString; pub use error::Error; /// Initialization impl<'repo> Remote<'repo> { pub(crate) fn from_preparsed_config( - name: Option, + name_or_url: Option, url: Option, push_url: Option, fetch_specs: Vec, @@ -44,7 +43,7 @@ impl<'repo> Remote<'repo> { .then(|| rewrite_urls(&repo.config, url.as_ref(), push_url.as_ref())) .unwrap_or(Ok((None, None)))?; Ok(Remote { - name: name.map(remote::name::validated).transpose()?, + name: name_or_url.map(Into::into), url, url_alias, push_url, diff --git a/git-repository/src/remote/mod.rs b/git-repository/src/remote/mod.rs index b83874858c5..9aae7dce4d5 100644 --- a/git-repository/src/remote/mod.rs +++ b/git-repository/src/remote/mod.rs @@ -1,3 +1,6 @@ +use crate::bstr::BStr; +use std::borrow::Cow; + /// The direction of an operation carried out (or to be carried out) through a remote. #[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] pub enum Direction { @@ -17,35 +20,24 @@ impl Direction { } } +/// The name of a remote, either interpreted as symbol like `origin` or as url as returned by [`Remote::name()`][crate::Remote::name()]. +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum Name<'repo> { + /// A symbolic name, like `origin`. + /// Note that it has not necessarily been validated yet. + Symbol(Cow<'repo, str>), + /// A url pointing to the remote host directly. + Url(Cow<'repo, BStr>), +} + +/// +pub mod name; + mod build; mod errors; pub use errors::find; -/// -pub mod name { - /// The error returned by [validated()]. - #[derive(Debug, thiserror::Error)] - #[error("remote names must be valid within refspecs for fetching: {name:?}")] - #[allow(missing_docs)] - pub struct Error { - source: git_refspec::parse::Error, - name: String, - } - - /// Return `name` if it is valid or convert it into an `Error`. - pub fn validated(name: impl Into) -> Result { - let name = name.into(); - match git_refspec::parse( - format!("refs/heads/test:refs/remotes/{name}/test").as_str().into(), - git_refspec::parse::Operation::Fetch, - ) { - Ok(_) => Ok(name), - Err(err) => Err(Error { source: err, name }), - } - } -} - /// pub mod init; diff --git a/git-repository/src/reference/remote/name.rs b/git-repository/src/remote/name.rs similarity index 62% rename from git-repository/src/reference/remote/name.rs rename to git-repository/src/remote/name.rs index 1137006bba1..7a178e0c318 100644 --- a/git-repository/src/reference/remote/name.rs +++ b/git-repository/src/remote/name.rs @@ -1,7 +1,28 @@ use std::{borrow::Cow, convert::TryFrom}; use super::Name; -use crate::bstr::{BStr, ByteSlice, ByteVec}; +use crate::bstr::{BStr, BString, ByteSlice, ByteVec}; + +/// The error returned by [validated()]. +#[derive(Debug, thiserror::Error)] +#[error("remote names must be valid within refspecs for fetching: {name:?}")] +#[allow(missing_docs)] +pub struct Error { + source: git_refspec::parse::Error, + name: BString, +} + +/// Return `name` if it is valid or convert it into an `Error`. +pub fn validated(name: impl Into) -> Result { + let name = name.into(); + match git_refspec::parse( + format!("refs/heads/test:refs/remotes/{name}/test").as_str().into(), + git_refspec::parse::Operation::Fetch, + ) { + Ok(_) => Ok(name), + Err(err) => Err(Error { source: err, name }), + } +} impl Name<'_> { /// Obtain the name as string representation. @@ -48,6 +69,12 @@ impl<'a> TryFrom> for Name<'a> { } } +impl From for Name<'static> { + fn from(name: BString) -> Self { + Self::try_from(Cow::Owned(name)).expect("String is never illformed") + } +} + impl<'a> AsRef for Name<'a> { fn as_ref(&self) -> &BStr { self.as_bstr() diff --git a/git-repository/src/remote/save.rs b/git-repository/src/remote/save.rs index d6c0e885d09..ebc01cb86f4 100644 --- a/git-repository/src/remote/save.rs +++ b/git-repository/src/remote/save.rs @@ -1,3 +1,4 @@ +use crate::bstr::BString; use std::convert::TryInto; use crate::Remote; @@ -39,7 +40,7 @@ impl Remote<'_> { .to_owned(), })?; if let Some(section_ids) = config.sections_and_ids_by_name("remote").map(|it| { - it.filter_map(|(s, id)| (s.header().subsection_name() == Some(name.into())).then(|| id)) + it.filter_map(|(s, id)| (s.header().subsection_name() == Some(name.as_bstr())).then(|| id)) .collect::>() }) { let mut sections_to_remove = Vec::new(); @@ -62,7 +63,7 @@ impl Remote<'_> { } } let mut section = config - .section_mut_or_create_new("remote", Some(name)) + .section_mut_or_create_new("remote", Some(name.as_ref())) .expect("section name is validated and 'remote' is acceptable"); if let Some(url) = self.url.as_ref() { section.push("url".try_into().expect("valid"), Some(url.to_bstring().as_ref())) @@ -91,12 +92,12 @@ impl Remote<'_> { /// and the caller should account for that. pub fn save_as_to( &mut self, - name: impl Into, + name: impl Into, config: &mut git_config::File<'static>, ) -> Result<(), AsError> { let name = crate::remote::name::validated(name)?; let prev_name = self.name.take(); - self.name = name.into(); + self.name = Some(name.into()); self.save_to(config).map_err(|err| { self.name = prev_name; err.into() diff --git a/git-repository/src/repository/config/mod.rs b/git-repository/src/repository/config/mod.rs index 8ce0df414bc..bb887254c43 100644 --- a/git-repository/src/repository/config/mod.rs +++ b/git-repository/src/repository/config/mod.rs @@ -44,6 +44,7 @@ mod remote { impl crate::Repository { /// Returns a sorted list unique of symbolic names of remotes that /// we deem [trustworthy][crate::open::Options::filter_config_section()]. + // TODO: Use `remote::Name` here pub fn remote_names(&self) -> BTreeSet<&str> { self.subsection_names_of("remote") } @@ -56,6 +57,7 @@ mod remote { /// # Notes /// /// It's up to the caller to determine what to do if the current `head` is unborn or detached. + // TODO: use remote::Name here pub fn remote_default_name(&self, direction: remote::Direction) -> Option> { let name = (direction == remote::Direction::Push) .then(|| { @@ -83,6 +85,7 @@ mod remote { mod branch { use std::{borrow::Cow, collections::BTreeSet, convert::TryInto}; + use crate::bstr::BStr; use git_ref::FullNameRef; use git_validate::reference::name::Error as ValidateNameError; @@ -99,13 +102,13 @@ mod branch { /// The returned reference is the one we track on the remote side for merging and pushing. /// Returns `None` if the remote reference was not found. /// May return an error if the reference is invalid. - pub fn branch_remote_ref( + pub fn branch_remote_ref<'a>( &self, - short_branch_name: &str, + short_branch_name: impl Into<&'a BStr>, ) -> Option, ValidateNameError>> { self.config .resolved - .string("branch", Some(short_branch_name), "merge") + .string("branch", Some(short_branch_name.into()), "merge") .map(|v| match v { Cow::Borrowed(v) => v.try_into().map(Cow::Borrowed), Cow::Owned(v) => v.try_into().map(Cow::Owned), @@ -119,10 +122,13 @@ mod branch { /// /// See also [Reference::remote_name()][crate::Reference::remote_name()] for a more typesafe version /// to be used when a `Reference` is available. - pub fn branch_remote_name(&self, short_branch_name: &str) -> Option> { + pub fn branch_remote_name<'a>( + &self, + short_branch_name: impl Into<&'a BStr>, + ) -> Option> { self.config .resolved - .string("branch", Some(short_branch_name), "remote") + .string("branch", Some(short_branch_name.into()), "remote") .and_then(|name| name.try_into().ok()) } } diff --git a/git-repository/src/repository/remote.rs b/git-repository/src/repository/remote.rs index c23fbb10b68..0a838ceab36 100644 --- a/git-repository/src/repository/remote.rs +++ b/git-repository/src/repository/remote.rs @@ -1,5 +1,6 @@ use std::convert::TryInto; +use crate::bstr::BStr; use crate::{remote, remote::find, Remote}; impl crate::Repository { @@ -23,13 +24,16 @@ impl crate::Repository { Remote::from_fetch_url(url, false, self) } - /// Find the remote with the given `name` or report an error, similar to [`try_find_remote(…)`][Self::try_find_remote()]. + /// Find the remote with the given `name_or_url` or report an error, similar to [`try_find_remote(…)`][Self::try_find_remote()]. /// - /// Note that we will include remotes only if we deem them [trustworthy][crate::open::Options::filter_config_section()]. - pub fn find_remote(&self, name: &str) -> Result, find::existing::Error> { + /// Note that we will obtain remotes only if we deem them [trustworthy][crate::open::Options::filter_config_section()]. + pub fn find_remote<'a>(&self, name_or_url: impl Into<&'a BStr>) -> Result, find::existing::Error> { + let name_or_url = name_or_url.into(); Ok(self - .try_find_remote(name) - .ok_or_else(|| find::existing::Error::NotFound { name: name.into() })??) + .try_find_remote(name_or_url) + .ok_or_else(|| find::existing::Error::NotFound { + name: name_or_url.into(), + })??) } /// Find the default remote as configured, or `None` if no such configuration could be found. @@ -43,7 +47,7 @@ impl crate::Repository { .map(|name| self.find_remote(name.as_ref())) } - /// Find the remote with the given `name` or return `None` if it doesn't exist, for the purpose of fetching or pushing + /// Find the remote with the given `name_or_url` or return `None` if it doesn't exist, for the purpose of fetching or pushing /// data to a remote. /// /// There are various error kinds related to partial information or incorrectly formatted URLs or ref-specs. @@ -53,23 +57,31 @@ impl crate::Repository { /// as negations/excludes are applied after includes. /// /// We will only include information if we deem it [trustworthy][crate::open::Options::filter_config_section()]. - pub fn try_find_remote(&self, name: &str) -> Option, find::Error>> { - self.try_find_remote_inner(name, true) + pub fn try_find_remote<'a>(&self, name_or_url: impl Into<&'a BStr>) -> Option, find::Error>> { + self.try_find_remote_inner(name_or_url, true) } /// Similar to [try_find_remote()][Self::try_find_remote()], but removes a failure mode if rewritten URLs turn out to be invalid /// as it skips rewriting them. /// Use this in conjunction with [`Remote::rewrite_urls()`] to non-destructively apply the rules and keep the failed urls unchanged. - pub fn try_find_remote_without_url_rewrite(&self, name: &str) -> Option, find::Error>> { - self.try_find_remote_inner(name, false) + pub fn try_find_remote_without_url_rewrite<'a>( + &self, + name_or_url: impl Into<&'a BStr>, + ) -> Option, find::Error>> { + self.try_find_remote_inner(name_or_url, false) } - fn try_find_remote_inner(&self, name: &str, rewrite_urls: bool) -> Option, find::Error>> { + fn try_find_remote_inner<'a>( + &self, + name_or_url: impl Into<&'a BStr>, + rewrite_urls: bool, + ) -> Option, find::Error>> { let mut filter = self.filter_config_section(); + let name_or_url = name_or_url.into(); let mut config_url = |field: &str, kind: &'static str| { self.config .resolved - .string_filter("remote", name.into(), field, &mut filter) + .string_filter("remote", Some(name_or_url), field, &mut filter) .map(|url| { git_url::parse::parse(url.as_ref()).map_err(|err| find::Error::Url { kind, @@ -88,7 +100,7 @@ impl crate::Repository { }; self.config .resolved - .strings_filter("remote", name.into(), kind, &mut filter) + .strings_filter("remote", Some(name_or_url), kind, &mut filter) .map(|specs| { specs .into_iter() @@ -139,7 +151,7 @@ impl crate::Repository { Some( Remote::from_preparsed_config( - name.to_owned().into(), + Some(name_or_url.to_owned()), url, push_url, fetch_specs, diff --git a/git-repository/src/types.rs b/git-repository/src/types.rs index a7c3ae701f7..16b658659ce 100644 --- a/git-repository/src/types.rs +++ b/git-repository/src/types.rs @@ -2,7 +2,7 @@ use std::{cell::RefCell, path::PathBuf}; use git_hash::ObjectId; -use crate::head; +use crate::{head, remote}; /// The kind of repository. #[derive(Debug, Clone, Eq, PartialEq, Ord, PartialOrd, Hash)] @@ -181,7 +181,7 @@ pub struct ThreadSafeRepository { #[derive(Debug, Clone, PartialEq)] pub struct Remote<'repo> { /// The remotes symbolic name, only present if persisted in git configuration files. - pub(crate) name: Option, + pub(crate) name: Option>, /// The url of the host to talk to, after application of replacements. If it is unset, the `push_url` must be set. /// and fetches aren't possible. pub(crate) url: Option, diff --git a/git-repository/tests/clone/mod.rs b/git-repository/tests/clone/mod.rs index cb679219404..51ce0253f38 100644 --- a/git-repository/tests/clone/mod.rs +++ b/git-repository/tests/clone/mod.rs @@ -141,16 +141,14 @@ mod blocking_io { "local clone always adopts the name of the remote" ); - let short_name = referent.name().shorten().to_str_lossy(); + let short_name = referent.name().shorten(); assert_eq!( - repo.branch_remote_name(short_name.as_ref()) - .expect("remote is set") - .as_ref(), + repo.branch_remote_name(short_name).expect("remote is set").as_ref(), remote_name, "the remote branch information is fully configured" ); assert_eq!( - repo.branch_remote_ref(short_name.as_ref()).expect("present")?.as_bstr(), + repo.branch_remote_ref(short_name).expect("present")?.as_bstr(), "refs/heads/main" ); diff --git a/git-repository/tests/reference/remote.rs b/git-repository/tests/reference/remote.rs index fc2fea7a9a3..7fe0e15137c 100644 --- a/git-repository/tests/reference/remote.rs +++ b/git-repository/tests/reference/remote.rs @@ -19,14 +19,16 @@ fn push_defaults_to_fetch() -> crate::Result { .remote(git::remote::Direction::Push) .expect("configured")? .name() - .expect("set"), + .expect("set") + .as_bstr(), "origin" ); assert_eq!( head.into_remote(git::remote::Direction::Push) .expect("same with branch")? .name() - .expect("set"), + .expect("set") + .as_bstr(), "origin" ); Ok(()) diff --git a/git-repository/tests/remote/save.rs b/git-repository/tests/remote/save.rs index 78c885ce1d1..e5d224785c6 100644 --- a/git-repository/tests/remote/save.rs +++ b/git-repository/tests/remote/save.rs @@ -23,7 +23,7 @@ mod save_to { let previous_remote_state = repo .config_snapshot() .plumbing() - .section("remote", Some("origin")) + .section_by_key("remote.origin") .expect("present") .to_bstring(); let mut config = repo.config_snapshot().plumbing().clone(); @@ -34,7 +34,7 @@ mod save_to { "amount of remotes are unaltered" ); assert_eq!( - config.section("remote", Some("origin")).expect("present").to_bstring(), + config.section_by_key("remote.origin").expect("present").to_bstring(), previous_remote_state, "the serialization doesn't modify anything" ); @@ -96,11 +96,11 @@ mod save_as_to { new_section.push("a".try_into().unwrap(), Some("value".into())); config - .section_mut_or_create_new("initially-empty-not-removed", Some("name")) + .section_mut_or_create_new("initially-empty-not-removed", Some("name".into())) .expect("works"); let mut existing_section = config - .section_mut_or_create_new("remote", Some("origin")) + .section_mut_or_create_new("remote", Some("origin".into())) .expect("works"); existing_section.push("free".try_into().unwrap(), Some("should not be removed".into())) } diff --git a/git-repository/tests/repository/config/config_snapshot/mod.rs b/git-repository/tests/repository/config/config_snapshot/mod.rs index 354f3a6ad90..20884cd4814 100644 --- a/git-repository/tests/repository/config/config_snapshot/mod.rs +++ b/git-repository/tests/repository/config/config_snapshot/mod.rs @@ -58,7 +58,9 @@ fn values_are_set_in_memory_only() { { let mut config = repo.config_snapshot_mut(); config.set_raw_value("hallo", None, "welt", "true").unwrap(); - config.set_raw_value("hallo", Some("unter"), "welt", "value").unwrap(); + config + .set_raw_value("hallo", Some("unter".into()), "welt", "value") + .unwrap(); } assert_eq!( diff --git a/git-repository/tests/repository/remote.rs b/git-repository/tests/repository/remote.rs index 4028a0b8e9b..6e060f0d924 100644 --- a/git-repository/tests/repository/remote.rs +++ b/git-repository/tests/repository/remote.rs @@ -121,7 +121,7 @@ mod find_remote { for (name, (url, refspec)) in repo.remote_names().into_iter().zip(expected) { count += 1; let remote = repo.find_remote(name).expect("no error"); - assert_eq!(remote.name(), Some(name)); + assert_eq!(remote.name().expect("set").as_bstr(), name); let url = git::url::parse(url.into()).expect("valid"); assert_eq!(remote.url(Direction::Fetch).unwrap(), &url); @@ -285,7 +285,8 @@ mod find_default_remote { .transpose()? .expect("present") .name() - .expect("always named"), + .expect("always named") + .as_bstr(), "origin" ); Ok(()) From 1c2e755e517b0f9fe8671187f5c30076ce43a3c9 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 14:29:18 +0100 Subject: [PATCH 05/29] adapt to changes in `git-config` --- cargo-smart-release/src/command/release/git.rs | 4 +++- gitoxide-core/src/organize.rs | 2 +- gitoxide-core/src/repository/config.rs | 14 +++++++------- src/plumbing/options/mod.rs | 6 +++++- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/cargo-smart-release/src/command/release/git.rs b/cargo-smart-release/src/command/release/git.rs index 0478c0d2880..91b5825a2b9 100644 --- a/cargo-smart-release/src/command/release/git.rs +++ b/cargo-smart-release/src/command/release/git.rs @@ -100,7 +100,9 @@ pub fn push_tags_and_head( .into_remote(git::remote::Direction::Push) .ok_or_else(|| anyhow!("Cannot push in uninitialized repo"))?? .name() - .expect("configured remotes have a name"), + .expect("configured remotes have a name") + .as_bstr() + .to_string(), ) .arg("HEAD"); for tag_name in tag_names { diff --git a/gitoxide-core/src/organize.rs b/gitoxide-core/src/organize.rs index 61fa2f9e12b..22dbbabd741 100644 --- a/gitoxide-core/src/organize.rs +++ b/gitoxide-core/src/organize.rs @@ -104,7 +104,7 @@ fn find_origin_remote(repo: &Path) -> anyhow::Result> { let config = git::config::File::from_path_no_includes(non_bare.as_path(), local) .or_else(|_| git::config::File::from_path_no_includes(repo.join("config").as_path(), local))?; Ok(config - .string("remote", Some("origin"), "url") + .string_by_key("remote.origin.url") .map(|url| git_url::Url::from_bytes(url.as_ref())) .transpose()?) } diff --git a/gitoxide-core/src/repository/config.rs b/gitoxide-core/src/repository/config.rs index 8dc4680ee25..11bfb43fe42 100644 --- a/gitoxide-core/src/repository/config.rs +++ b/gitoxide-core/src/repository/config.rs @@ -1,12 +1,12 @@ use anyhow::{bail, Result}; +use git::bstr::{BStr, BString}; use git_repository as git; -use git_repository::bstr::BString; use crate::OutputFormat; pub fn list( repo: git::Repository, - filters: Vec, + filters: Vec, overrides: Vec, format: OutputFormat, mut out: impl std::io::Write, @@ -52,18 +52,18 @@ pub fn list( struct Filter { name: String, - subsection: Option, + subsection: Option, } impl Filter { - fn new(input: String) -> Self { - match git::config::parse::key(&input) { + fn new(input: BString) -> Self { + match git::config::parse::key(<_ as AsRef>::as_ref(&input)) { Some(key) => Filter { name: key.section_name.into(), subsection: key.subsection_name.map(ToOwned::to_owned), }, None => Filter { - name: input, + name: input.to_string(), subsection: None, }, } @@ -77,7 +77,7 @@ impl Filter { } match (self.subsection.as_deref(), section.header().subsection_name()) { (Some(filter), Some(name)) => { - if !git::glob::wildmatch(filter.as_bytes().into(), name, ignore_case) { + if !git::glob::wildmatch(filter.as_slice().into(), name, ignore_case) { return false; } } diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index 7e4ace87e78..c7cfc1b0509 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -106,6 +106,9 @@ pub enum Subcommands { } pub mod config { + use git::bstr::BString; + use git_repository as git; + /// Print all entries in a configuration file or access other sub-commands #[derive(Debug, clap::Parser)] #[clap(subcommand_required(false))] @@ -114,7 +117,8 @@ pub mod config { /// /// Typical filters are `branch` or `remote.origin` or `remote.or*` - git-style globs are supported /// and comparisons are case-insensitive. - pub filter: Vec, + #[clap(parse(try_from_os_str = git::env::os_str_to_bstring))] + pub filter: Vec, } } From 340dcad91832668bc1b570f35714178aa2c53ece Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 09:32:05 +0100 Subject: [PATCH 06/29] Allow remote overrides for http options --- git-repository/src/config/mod.rs | 11 +- .../src/remote/connection/ref_map.rs | 16 +- .../src/repository/config/transport.rs | 139 ++++++++++++------ .../make_config_repos.tar.xz | 4 +- .../tests/fixtures/make_config_repos.sh | 10 ++ .../repository/config/transport_options.rs | 37 +++-- 6 files changed, 147 insertions(+), 70 deletions(-) diff --git a/git-repository/src/config/mod.rs b/git-repository/src/config/mod.rs index e148ebacbb4..88ffb6970f4 100644 --- a/git-repository/src/config/mod.rs +++ b/git-repository/src/config/mod.rs @@ -110,6 +110,8 @@ pub mod checkout_options { /// pub mod transport { use crate::bstr; + use crate::bstr::BStr; + use std::borrow::Cow; /// The error produced when configuring a transport for a particular protocol. #[derive(Debug, thiserror::Error)] @@ -130,7 +132,7 @@ pub mod transport { }, #[error("Could not decode value at key {key:?} as UTF-8 string")] IllformedUtf8 { - key: &'static str, + key: Cow<'static, BStr>, source: bstr::FromUtf8Error, }, #[error("Invalid URL passed for configuration")] @@ -141,12 +143,15 @@ pub mod transport { /// pub mod http { + use crate::bstr::BStr; + use std::borrow::Cow; + /// The error produced when configuring a HTTP transport. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { - #[error("The proxy authentication method name {value:?} is invalid")] - InvalidProxyAuthMethod { value: String }, + #[error("The proxy authentication method name {value:?} found at key `{key}` is invalid")] + InvalidProxyAuthMethod { value: String, key: Cow<'static, BStr> }, #[error("Could not configure the credential helpers for the authenticated proxy url")] ConfigureProxyAuthenticate(#[from] crate::config::snapshot::credential_helpers::Error), } diff --git a/git-repository/src/remote/connection/ref_map.rs b/git-repository/src/remote/connection/ref_map.rs index 727376b8f21..566bac62591 100644 --- a/git-repository/src/remote/connection/ref_map.rs +++ b/git-repository/src/remote/connection/ref_map.rs @@ -161,14 +161,14 @@ where }; if self.transport_options.is_none() { - self.transport_options = - self.remote - .repo - .transport_options(url.as_ref()) - .map_err(|err| Error::GatherTransportConfig { - source: err, - url: url.into_owned(), - })?; + self.transport_options = self + .remote + .repo + .transport_options(url.as_ref(), self.remote.name().map(|n| n.as_bstr())) + .map_err(|err| Error::GatherTransportConfig { + source: err, + url: url.into_owned(), + })?; } if let Some(config) = self.transport_options.as_ref() { self.transport.configure(&**config)?; diff --git a/git-repository/src/repository/config/transport.rs b/git-repository/src/repository/config/transport.rs index 6243c8491b3..59dff785696 100644 --- a/git-repository/src/repository/config/transport.rs +++ b/git-repository/src/repository/config/transport.rs @@ -5,19 +5,25 @@ use crate::bstr::BStr; impl crate::Repository { /// Produce configuration suitable for `url`, as differentiated by its protocol/scheme, to be passed to a transport instance via /// [configure()][git_transport::client::TransportWithoutIO::configure()] (via `&**config` to pass the contained `Any` and not the `Box`). - /// `None` is returned if there is no known configuration. + /// `None` is returned if there is no known configuration. If `remote_name` is not `None`, the remote's name may contribute to + /// configuration overrides, typically for the HTTP transport. /// /// Note that the caller may cast the instance themselves to modify it before passing it on. /// - /// - // let (mut cascade, _action_with_normalized_url, prompt_opts) = - // self.remote.repo.config_snapshot().credential_helpers(url)?; - // Ok(Box::new(move |action| cascade.invoke(action, prompt_opts.clone())) as AuthenticateFn<'_>) - /// For transports that support proxy authentication, the authentication - /// [default authentication method](crate::config::Snapshot::credential_helpers()) will be used with the url of the proxy. + /// For transports that support proxy authentication, the + /// [default authentication method](crate::config::Snapshot::credential_helpers()) will be used with the url of the proxy + /// if it contains a user name. + #[cfg_attr( + not(any( + feature = "blocking-http-transport-reqwest", + feature = "blocking-http-transport-curl" + )), + allow(unused_variables) + )] pub fn transport_options<'a>( &self, url: impl Into<&'a BStr>, + remote_name: Option<&BStr>, ) -> Result>, crate::config::transport::Error> { let url = git_url::parse(url.into())?; use git_url::Scheme::*; @@ -49,12 +55,15 @@ impl crate::Repository { fn try_cow_to_string( v: Cow<'_, BStr>, lenient: bool, - key: &'static str, + key: impl Into>, ) -> Result, crate::config::transport::Error> { Vec::from(v.into_owned()) .into_string() .map(Some) - .map_err(|err| crate::config::transport::Error::IllformedUtf8 { source: err, key }) + .map_err(|err| crate::config::transport::Error::IllformedUtf8 { + source: err, + key: key.into(), + }) .with_leniency(lenient) } @@ -71,6 +80,7 @@ impl crate::Repository { { Ok(integer_opt(config, lenient, key, kind, filter)?.unwrap_or(default)) } + fn integer_opt( config: &git_config::File<'static>, lenient: bool, @@ -103,6 +113,55 @@ impl crate::Repository { .transpose() .with_leniency(lenient) } + + fn proxy_auth_method( + value_and_key: Option<(Cow<'_, BStr>, Cow<'static, BStr>)>, + lenient: bool, + ) -> Result { + let value = value_and_key + .and_then(|(v, k)| { + try_cow_to_string(v, lenient, k.clone()) + .map(|v| v.map(|v| (v, k))) + .transpose() + }) + .transpose()? + .map(|(method, key)| { + Ok(match method.as_str() { + "anyauth" => ProxyAuthMethod::AnyAuth, + "basic" => ProxyAuthMethod::Basic, + "digest" => ProxyAuthMethod::Digest, + "negotiate" => ProxyAuthMethod::Negotiate, + "ntlm" => ProxyAuthMethod::Ntlm, + _ => { + return Err(crate::config::transport::http::Error::InvalidProxyAuthMethod { + value: method, + key, + }) + } + }) + }) + .transpose()? + .unwrap_or_default(); + Ok(value) + } + + fn proxy( + value: Option<(Cow<'_, BStr>, Cow<'static, BStr>)>, + lenient: bool, + ) -> Result, crate::config::transport::Error> { + Ok(value + .and_then(|(v, k)| try_cow_to_string(v, lenient, k.clone()).transpose()) + .transpose()? + .map(|mut proxy| { + if !proxy.trim().is_empty() && !proxy.contains("://") { + proxy.insert_str(0, "http://"); + proxy + } else { + proxy + } + })) + } + let mut opts = http::Options::default(); let config = &self.config.resolved; let mut trusted_only = self.filter_config_section(); @@ -113,7 +172,7 @@ impl crate::Repository { .strings_filter("http", None, "extraHeader", &mut trusted_only) .unwrap_or_default() .into_iter() - .map(|v| try_cow_to_string(v, lenient, "http.extraHeader")) + .map(|v| try_cow_to_string(v, lenient, Cow::Borrowed("http.extraHeader".into()))) { let header = header?; if let Some(header) = header { @@ -149,38 +208,34 @@ impl crate::Repository { integer(config, lenient, "http.lowSpeedTime", "u64", trusted_only, 0)?; opts.low_speed_limit_bytes_per_second = integer(config, lenient, "http.lowSpeedLimit", "u32", trusted_only, 0)?; - opts.proxy = config - .string_filter("http", None, "proxy", &mut trusted_only) - .and_then(|v| try_cow_to_string(v, lenient, "http.proxy").transpose()) - .transpose()? - .map(|mut proxy| { - if !proxy.trim().is_empty() && !proxy.contains("://") { - proxy.insert_str(0, "http://"); - proxy - } else { - proxy - } - }); - opts.proxy_auth_method = config - .string_filter("http", None, "proxyAuthMethod", &mut trusted_only) - .and_then(|v| try_cow_to_string(v, lenient, "http.proxyAuthMethod").transpose()) - .transpose()? - .map(|method| { - Ok(match method.as_str() { - "anyauth" => ProxyAuthMethod::AnyAuth, - "basic" => ProxyAuthMethod::Basic, - "digest" => ProxyAuthMethod::Digest, - "negotiate" => ProxyAuthMethod::Negotiate, - "ntlm" => ProxyAuthMethod::Ntlm, - _ => { - return Err(crate::config::transport::http::Error::InvalidProxyAuthMethod { - value: method, - }) - } + opts.proxy = proxy( + remote_name + .and_then(|name| { + config + .string_filter("remote", Some(name), "proxy", &mut trusted_only) + .map(|v| (v, Cow::Owned(format!("remote.{name}.proxy").into()))) }) - }) - .transpose()? - .unwrap_or_default(); + .or_else(|| { + config + .string_filter("http", None, "proxy", &mut trusted_only) + .map(|v| (v, Cow::Borrowed("http.proxy".into()))) + }), + lenient, + )?; + opts.proxy_auth_method = proxy_auth_method( + remote_name + .and_then(|name| { + config + .string_filter("remote", Some(name), "proxyAuthMethod", &mut trusted_only) + .map(|v| (v, Cow::Owned(format!("remote.{name}.proxyAuthMethod").into()))) + }) + .or_else(|| { + config + .string_filter("http", None, "proxyAuthMethod", &mut trusted_only) + .map(|v| (v, Cow::Borrowed("http.proxyAuthMethod".into()))) + }), + lenient, + )?; opts.proxy_authenticate = opts .proxy .as_deref() @@ -202,7 +257,7 @@ impl crate::Repository { .map(std::time::Duration::from_millis); opts.user_agent = config .string_filter("http", None, "userAgent", &mut trusted_only) - .and_then(|v| try_cow_to_string(v, lenient, "http.userAgent").transpose()) + .and_then(|v| try_cow_to_string(v, lenient, Cow::Borrowed("http.userAgent".into())).transpose()) .transpose()? .or_else(|| Some(crate::env::agent().into())); diff --git a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz index 180ecfb95c7..949420ac9ec 100644 --- a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz +++ b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:495e3613b7ee9c5a02101dd307de24d9754e531e2840bedecfed00d55ea656af -size 10252 +oid sha256:83f5270e487f0364e4a63f82cf4bfb5cb302bee744e9d3df521b7b3f850f3046 +size 10772 diff --git a/git-repository/tests/fixtures/make_config_repos.sh b/git-repository/tests/fixtures/make_config_repos.sh index 660393ac9f0..e7f514eb811 100644 --- a/git-repository/tests/fixtures/make_config_repos.sh +++ b/git-repository/tests/fixtures/make_config_repos.sh @@ -16,6 +16,16 @@ git init http-config git config gitoxide.http.connectTimeout 60k ) +git clone --shared http-config http-remote-override +(cd http-remote-override + + git config http.proxy http://localhost:9090 + git config http.proxyAuthMethod basic + + git config remote.origin.proxy overridden + git config remote.origin.proxyAuthMethod negotiate +) + git init http-proxy-empty (cd http-proxy-empty git config http.proxy localhost:9090 diff --git a/git-repository/tests/repository/config/transport_options.rs b/git-repository/tests/repository/config/transport_options.rs index f72451b7430..b551983535e 100644 --- a/git-repository/tests/repository/config/transport_options.rs +++ b/git-repository/tests/repository/config/transport_options.rs @@ -4,15 +4,16 @@ ))] mod http { use git_repository as git; + use git_transport::client::http::options::{FollowRedirects, ProxyAuthMethod}; pub(crate) fn repo(name: &str) -> git::Repository { let dir = git_testtools::scripted_fixture_repo_read_only("make_config_repos.sh").unwrap(); git::open_opts(dir.join(name), git::open::Options::isolated()).unwrap() } - fn http_options(repo: &git::Repository) -> git_transport::client::http::Options { + fn http_options(repo: &git::Repository, remote_name: Option<&str>) -> git_transport::client::http::Options { let opts = repo - .transport_options("https://example.com/does/not/matter") + .transport_options("https://example.com/does/not/matter", remote_name.map(Into::into)) .expect("valid configuration") .expect("configuration available for http"); opts.downcast_ref::() @@ -20,6 +21,19 @@ mod http { .to_owned() } + #[test] + fn remote_overrides() { + let repo = repo("http-remote-override"); + let git_transport::client::http::Options { + proxy, + proxy_auth_method, + .. + } = http_options(&repo, Some("origin")); + + assert_eq!(proxy_auth_method, ProxyAuthMethod::Negotiate); + assert_eq!(proxy.as_deref(), Some("http://overridden")); + } + #[test] fn simple_configuration() { let repo = repo("http-config"); @@ -34,16 +48,13 @@ mod http { user_agent, connect_timeout, backend, - } = http_options(&repo); + } = http_options(&repo, None); assert_eq!( extra_headers, &["ExtraHeader: value2", "ExtraHeader: value3"], "it respects empty values to clear prior values" ); - assert_eq!( - follow_redirects, - git_transport::client::http::options::FollowRedirects::Initial - ); + assert_eq!(follow_redirects, FollowRedirects::Initial); assert_eq!(low_speed_limit_bytes_per_second, 5120); assert_eq!(low_speed_time_seconds, 10); assert_eq!(proxy.as_deref(), Some("http://localhost:9090"),); @@ -51,11 +62,7 @@ mod http { proxy_authenticate.is_none(), "no username means no authentication required" ); - assert_eq!( - proxy_auth_method, - git_transport::client::http::options::ProxyAuthMethod::Basic, - "TODO: implement auth" - ); + assert_eq!(proxy_auth_method, ProxyAuthMethod::Basic, "TODO: implement auth"); assert_eq!(user_agent.as_deref(), Some("agentJustForHttp")); assert_eq!(connect_timeout, Some(std::time::Duration::from_millis(60 * 1024))); assert!( @@ -68,7 +75,7 @@ mod http { fn http_proxy_with_username() { let repo = repo("http-proxy-authenticated"); - let opts = http_options(&repo); + let opts = http_options(&repo, None); assert_eq!( opts.proxy.as_deref(), Some("http://user@localhost:9090"), @@ -84,7 +91,7 @@ mod http { fn empty_proxy_string_turns_it_off() { let repo = repo("http-proxy-empty"); - let opts = http_options(&repo); + let opts = http_options(&repo, None); assert_eq!( opts.proxy.as_deref(), Some(""), @@ -96,7 +103,7 @@ mod http { fn proxy_without_protocol_is_defaulted_to_http() { let repo = repo("http-proxy-auto-prefix"); - let opts = http_options(&repo); + let opts = http_options(&repo, None); assert_eq!(opts.proxy.as_deref(), Some("http://localhost:9090")); } } From 10f4f2149830734cc551ea96a3d087f07d43fe29 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 24 Nov 2022 14:40:12 +0100 Subject: [PATCH 07/29] thanks clippy --- git-config/src/file/util.rs | 1 - git-repository/src/clone/fetch/util.rs | 5 +---- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/git-config/src/file/util.rs b/git-config/src/file/util.rs index 6d3de0da41a..ac8424e923a 100644 --- a/git-config/src/file/util.rs +++ b/git-config/src/file/util.rs @@ -125,7 +125,6 @@ impl<'event> File<'event> { .ok_or(lookup::existing::Error::SectionMissing)?; let mut maybe_ids = None; if let Some(subsection_name) = subsection_name { - let subsection_name: &BStr = subsection_name.into(); for node in section_ids { if let SectionBodyIdsLut::NonTerminal(subsection_lookup) = node { maybe_ids = subsection_lookup.get(subsection_name).map(|v| v.iter().copied()); diff --git a/git-repository/src/clone/fetch/util.rs b/git-repository/src/clone/fetch/util.rs index 6f763041b3c..22aa26fa674 100644 --- a/git-repository/src/clone/fetch/util.rs +++ b/git-repository/src/clone/fetch/util.rs @@ -204,10 +204,7 @@ fn setup_branch_config( let mut section = config .new_section("branch", Some(Cow::Owned(short_name.into()))) .expect("section header name is always valid per naming rules, our input branch name is valid"); - section.push( - "remote".try_into().expect("valid at compile time"), - Some(remote_name.into()), - ); + section.push("remote".try_into().expect("valid at compile time"), Some(remote_name)); section.push( "merge".try_into().expect("valid at compile time"), Some(branch.as_bstr()), From 5c1abe7cc66f7c0a5a9466008e54c721fe67c569 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 25 Nov 2022 15:38:41 +0100 Subject: [PATCH 08/29] pledge to support sparse checkouts from day one --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index ef97fd31c4e..b7da4cbbfe9 100644 --- a/README.md +++ b/README.md @@ -256,6 +256,7 @@ Project goals can change over time as we learn more, and they can be challenged. * **be the best performing implementation** * use Rust's type system to optimize for work not done without being hard to use * make use of parallelism from the get go + * _sparse checkout_ support from day one * **assure on-disk consistency** * assure reads never interfere with concurrent writes * assure multiple concurrent writes don't cause trouble From 62cae0e6bfe8113c0225152a896338017c8de474 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 25 Nov 2022 15:59:42 +0100 Subject: [PATCH 09/29] don't lock stdout/stderr as it will deadlock on dbg-printing We should one day turn that back but then again, `gix` isn't outputting vast amounts of data to stdout, and shouldn't, as in most modes it's actually collected to memory first. Hence optimal write performance to stdout/stderr shouldn't be our priority here. --- src/shared.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/shared.rs b/src/shared.rs index f0765646ce1..b756f4ff5f5 100644 --- a/src/shared.rs +++ b/src/shared.rs @@ -113,13 +113,7 @@ pub mod pretty { crate::shared::init_env_logger(); match (verbose, progress) { - (false, false) => { - let stdout = stdout(); - let mut stdout_lock = stdout.lock(); - let stderr = stderr(); - let mut stderr_lock = stderr.lock(); - run(progress::DoOrDiscard::from(None), &mut stdout_lock, &mut stderr_lock) - } + (false, false) => run(progress::DoOrDiscard::from(None), &mut stdout(), &mut stderr()), (true, false) => { use crate::shared::{self, STANDARD_RANGE}; let progress = shared::progress_tree(); From 4ebe2ac05dc8bef3bc1783afca3fcfdc565c2aae Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 25 Nov 2022 18:15:20 +0100 Subject: [PATCH 10/29] doc: Actively discourage using `Boolean::try_from("")` explicitly. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Use `config.boolean(…)` instead. --- git-config-value/src/boolean.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/git-config-value/src/boolean.rs b/git-config-value/src/boolean.rs index 3e86f8e1255..64a9a429028 100644 --- a/git-config-value/src/boolean.rs +++ b/git-config-value/src/boolean.rs @@ -21,6 +21,15 @@ impl TryFrom for Boolean { } } +/// # Warning +/// +/// The direct usage of `try_from("string")` is discouraged as it will produce the wrong result for values +/// obtained from `core.bool-implicit-true`, which have no separator and are implicitly true. +/// This method chooses to work correctly for `core.bool-empty=`, which is an empty string and resolves +/// to being `false`. +/// +/// Instead of this, obtain booleans with `config.boolean(…)`, which handles the case were no separator is +/// present correctly. impl TryFrom<&BStr> for Boolean { type Error = Error; From 8e158c3f4056f59724fe91587157ef0daa517964 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 25 Nov 2022 17:48:33 +0100 Subject: [PATCH 11/29] feat!: interpret the FollowRedirects option for the curl HTTP backend. This comes with changes to the `HTTP` trait which now requires a base-url to be provided as well. --- .../src/client/blocking_io/http/curl/mod.rs | 26 ++++-- .../client/blocking_io/http/curl/remote.rs | 62 +++++++++----- .../src/client/blocking_io/http/mod.rs | 81 ++++++++++++++++++- .../client/blocking_io/http/reqwest/remote.rs | 7 +- .../src/client/blocking_io/http/traits.rs | 10 ++- 5 files changed, 153 insertions(+), 33 deletions(-) diff --git a/git-transport/src/client/blocking_io/http/curl/mod.rs b/git-transport/src/client/blocking_io/http/curl/mod.rs index f84555f1ea7..e8f29b28f31 100644 --- a/git-transport/src/client/blocking_io/http/curl/mod.rs +++ b/git-transport/src/client/blocking_io/http/curl/mod.rs @@ -1,5 +1,4 @@ use std::{ - error::Error, sync::mpsc::{Receiver, SyncSender}, thread, }; @@ -10,10 +9,20 @@ use crate::client::blocking_io::http; mod remote; +#[derive(Debug, thiserror::Error)] +pub enum Error { + #[error(transparent)] + Curl(#[from] curl::Error), + #[error(transparent)] + Redirect(#[from] http::redirect::Error), + #[error(transparent)] + Authenticate(#[from] git_credentials::protocol::Error), +} + pub struct Curl { req: SyncSender, res: Receiver, - handle: Option>>, + handle: Option>>, config: http::Options, } @@ -36,6 +45,7 @@ impl Curl { fn make_request( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, upload: bool, ) -> Result, http::Error> { @@ -47,6 +57,7 @@ impl Curl { .req .send(remote::Request { url: url.to_owned(), + base_url: base_url.to_owned(), headers: list, upload, config: self.config.clone(), @@ -92,20 +103,25 @@ impl http::Http for Curl { fn get( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, ) -> Result, http::Error> { - self.make_request(url, headers, false).map(Into::into) + self.make_request(url, base_url, headers, false).map(Into::into) } fn post( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, ) -> Result, http::Error> { - self.make_request(url, headers, true) + self.make_request(url, base_url, headers, true) } - fn configure(&mut self, config: &dyn std::any::Any) -> Result<(), Box> { + fn configure( + &mut self, + config: &dyn std::any::Any, + ) -> Result<(), Box> { if let Some(config) = config.downcast_ref::() { self.config = config.clone(); } diff --git a/git-transport/src/client/blocking_io/http/curl/remote.rs b/git-transport/src/client/blocking_io/http/curl/remote.rs index fe5bdf29551..e4c1a5db4a3 100644 --- a/git-transport/src/client/blocking_io/http/curl/remote.rs +++ b/git-transport/src/client/blocking_io/http/curl/remote.rs @@ -9,8 +9,8 @@ use std::{ use curl::easy::{Auth, Easy2}; use git_features::io::pipe; -use crate::client::blocking_io::http; -use crate::client::http::options::ProxyAuthMethod; +use crate::client::blocking_io::http::{self, curl::Error, redirect}; +use crate::client::http::options::{FollowRedirects, ProxyAuthMethod}; #[derive(Default)] struct Handler { @@ -19,6 +19,7 @@ struct Handler { receive_body: Option, checked_status: bool, last_status: usize, + follow: FollowRedirects, } impl Handler { @@ -34,9 +35,13 @@ impl Handler { let code = std::str::from_utf8(code)?; code.parse().map_err(Into::into) } - fn parse_status(data: &[u8]) -> Option<(usize, Box)> { + fn parse_status(data: &[u8], follow: FollowRedirects) -> Option<(usize, Box)> { + let valid_end = match follow { + FollowRedirects::Initial | FollowRedirects::All => 308, + FollowRedirects::None => 299, + }; match Self::parse_status_inner(data) { - Ok(status) if !(200..=299).contains(&status) => { + Ok(status) if !(200..=valid_end).contains(&status) => { Some((status, format!("Received HTTP status {}", status).into())) } Ok(_) => None, @@ -68,7 +73,7 @@ impl curl::easy::Handler for Handler { } else { self.checked_status = true; self.last_status = 200; - match Handler::parse_status(data) { + match Handler::parse_status(data, self.follow) { None => true, Some((status, err)) => { self.last_status = status; @@ -95,6 +100,7 @@ impl curl::easy::Handler for Handler { pub struct Request { pub url: String, + pub base_url: String, pub headers: curl::easy::List, pub upload: bool, pub config: http::Options, @@ -107,23 +113,26 @@ pub struct Response { } pub fn new() -> ( - thread::JoinHandle>, + thread::JoinHandle>, SyncSender, Receiver, ) { let (req_send, req_recv) = sync_channel(0); let (res_send, res_recv) = sync_channel(0); - let handle = std::thread::spawn(move || -> Result<(), curl::Error> { + let handle = std::thread::spawn(move || -> Result<(), Error> { let mut handle = Easy2::new(Handler::default()); + let mut follow = None; + let mut redirected_base_url = None::; for Request { url, + base_url, mut headers, upload, config: http::Options { extra_headers, - follow_redirects: _, + follow_redirects, low_speed_limit_bytes_per_second, low_speed_time_seconds, connect_timeout, @@ -135,7 +144,8 @@ pub fn new() -> ( }, } in req_recv { - handle.url(&url)?; + let effective_url = redirect::swap_tails(redirected_base_url.as_deref(), &base_url, url.clone()); + handle.url(&effective_url)?; // GitHub sends 'chunked' to avoid unknown clients to choke on the data, I suppose handle.post(upload)?; @@ -160,12 +170,7 @@ pub fn new() -> ( handle.proxy_type(proxy_type)?; if let Some((obtain_creds_action, authenticate)) = proxy_authenticate { - let creds = authenticate.lock().expect("no panics in other threads")(obtain_creds_action) - .map_err(|err| { - let mut e = curl::Error::new(0); - e.set_extra(err.to_string()); - e - })? + let creds = authenticate.lock().expect("no panics in other threads")(obtain_creds_action)? .expect("action to fetch credentials"); handle.proxy_username(&creds.identity.username)?; handle.proxy_password(&creds.identity.password)?; @@ -214,6 +219,14 @@ pub fn new() -> ( (receive_data, receive_headers, send_body) }; + let follow = follow.get_or_insert(follow_redirects); + handle.get_mut().follow = *follow; + handle.follow_location(matches!(*follow, FollowRedirects::Initial | FollowRedirects::All))?; + + if *follow == FollowRedirects::Initial { + *follow = FollowRedirects::None; + } + if res_send .send(Response { headers: receive_headers, @@ -256,17 +269,18 @@ pub fn new() -> ( action.store() } else { action.erase() - }) - .map_err(|err| { - let mut e = curl::Error::new(0); - e.set_extra(err.to_string()); - e })?; } handler.reset(); handler.receive_body.take(); handler.send_header.take(); handler.send_data.take(); + let actual_url = handle + .effective_url()? + .expect("effective url is present and valid UTF-8"); + if actual_url != effective_url { + redirected_base_url = redirect::base_url(actual_url, &base_url, url)?.into(); + } } } Ok(()) @@ -274,6 +288,14 @@ pub fn new() -> ( (handle, req_send, res_recv) } +impl From for http::Error { + fn from(err: Error) -> Self { + http::Error::Detail { + description: err.to_string(), + } + } +} + impl From for http::Error { fn from(err: curl::Error) -> Self { http::Error::Detail { diff --git a/git-transport/src/client/blocking_io/http/mod.rs b/git-transport/src/client/blocking_io/http/mod.rs index 5b01d333274..da779e51c71 100644 --- a/git-transport/src/client/blocking_io/http/mod.rs +++ b/git-transport/src/client/blocking_io/http/mod.rs @@ -252,7 +252,9 @@ impl client::TransportWithoutIO for Transport { headers, body, post_body, - } = self.http.post(&url, static_headers.iter().chain(&dynamic_headers))?; + } = self + .http + .post(&url, &self.url, static_headers.iter().chain(&dynamic_headers))?; let line_provider = self .line_provider .as_mut() @@ -319,9 +321,9 @@ impl client::Transport for Transport { dynamic_headers.push(format!("Git-Protocol: {}", parameters).into()); } self.add_basic_auth_if_present(&mut dynamic_headers)?; - let GetResponse { headers, body } = self - .http - .get(url.as_ref(), static_headers.iter().chain(&dynamic_headers))?; + let GetResponse { headers, body } = + self.http + .get(url.as_ref(), &self.url, static_headers.iter().chain(&dynamic_headers))?; >::check_content_type(service, "advertisement", headers)?; let line_reader = self @@ -431,3 +433,74 @@ pub fn connect_http(http: H, url: &str, desired_version: Protocol) -> T pub fn connect(url: &str, desired_version: Protocol) -> Transport { Transport::new(url, desired_version) } + +/// +#[cfg(feature = "http-client-curl")] +pub mod redirect { + /// The error provided when redirection went beyond what we deem acceptable. + #[derive(Debug, thiserror::Error)] + #[error("Redirect url {redirect_url:?} could not be reconciled with original url {expected_url} as they don't share the same suffix")] + pub struct Error { + redirect_url: String, + expected_url: String, + } + + pub(crate) fn base_url(redirect_url: &str, base_url: &str, url: String) -> Result { + let tail = url + .strip_prefix(base_url) + .expect("BUG: caller assures `base_url` is subset of `url`"); + redirect_url + .strip_suffix(tail) + .ok_or_else(|| Error { + redirect_url: redirect_url.into(), + expected_url: url, + }) + .map(ToOwned::to_owned) + } + + pub(crate) fn swap_tails(effective_base_url: Option<&str>, base_url: &str, mut url: String) -> String { + match effective_base_url { + Some(effective_base) => { + url.replace_range(..base_url.len(), effective_base); + url + } + None => url, + } + } + + #[cfg(test)] + mod tests { + use super::*; + + #[test] + fn base_url_complete() { + assert_eq!( + base_url( + "https://redirected.org/b/info/refs?hi", + "https://original/a", + "https://original/a/info/refs?hi".into() + ) + .unwrap(), + "https://redirected.org/b" + ); + } + + #[test] + fn swap_tails_complete() { + assert_eq!( + swap_tails(None, "not interesting", "used".into()), + "used", + "without effective base url, it passes url, no redirect happened yet" + ); + assert_eq!( + swap_tails( + Some("https://redirected.org/b"), + "https://original/a", + "https://original/a/info/refs?something".into() + ), + "https://redirected.org/b/info/refs?something", + "the tail stays the same if redirection happened" + ) + } + } +} diff --git a/git-transport/src/client/blocking_io/http/reqwest/remote.rs b/git-transport/src/client/blocking_io/http/reqwest/remote.rs index d0341bd1705..9eaeb0e2272 100644 --- a/git-transport/src/client/blocking_io/http/reqwest/remote.rs +++ b/git-transport/src/client/blocking_io/http/reqwest/remote.rs @@ -118,6 +118,7 @@ impl Remote { fn make_request( &mut self, url: &str, + _base_url: &str, headers: impl IntoIterator>, upload: bool, ) -> Result, http::Error> { @@ -182,17 +183,19 @@ impl http::Http for Remote { fn get( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, ) -> Result, http::Error> { - self.make_request(url, headers, false).map(Into::into) + self.make_request(url, base_url, headers, false).map(Into::into) } fn post( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, ) -> Result, http::Error> { - self.make_request(url, headers, true) + self.make_request(url, base_url, headers, true) } fn configure(&mut self, config: &dyn Any) -> Result<(), Box> { diff --git a/git-transport/src/client/blocking_io/http/traits.rs b/git-transport/src/client/blocking_io/http/traits.rs index 21ff84edd8d..6ab00dc2947 100644 --- a/git-transport/src/client/blocking_io/http/traits.rs +++ b/git-transport/src/client/blocking_io/http/traits.rs @@ -53,16 +53,21 @@ pub trait Http { /// A type allowing to write the content to post. type PostBody: std::io::Write; - /// Initiate a `GET` request to `url` provided the given `headers`. + /// Initiate a `GET` request to `url` provided the given `headers`, where `base_url` is so that `base_url + tail == url`. + /// + /// The `base_url` helps to validate redirects and to swap it with the effective base after a redirect. /// /// The `headers` are provided verbatim and include both the key as well as the value. fn get( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, ) -> Result, Error>; - /// Initiate a `POST` request to `url` providing with the given `headers`. + /// Initiate a `POST` request to `url` providing with the given `headers`, where `base_url` is so that `base_url + tail == url`. + /// + /// The `base_url` helps to validate redirects and to swap it with the effective base after a redirect. /// /// The `headers` are provided verbatim and include both the key as well as the value. /// Note that the [`PostResponse`] contains the [`post_body`][PostResponse::post_body] field which implements [`std::io::Write`] @@ -71,6 +76,7 @@ pub trait Http { fn post( &mut self, url: &str, + base_url: &str, headers: impl IntoIterator>, ) -> Result, Error>; From b84ae6a94082876bfc23cda167aabea88fda67be Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 25 Nov 2022 17:49:35 +0100 Subject: [PATCH 12/29] more faithfully parse http.followRedirect --- .../src/repository/config/transport.rs | 39 ++++++++++--------- .../make_config_repos.tar.xz | 4 +- .../tests/fixtures/make_config_repos.sh | 10 ++++- .../repository/config/transport_options.rs | 13 ++++++- 4 files changed, 42 insertions(+), 24 deletions(-) diff --git a/git-repository/src/repository/config/transport.rs b/git-repository/src/repository/config/transport.rs index 59dff785696..7b07ed83d45 100644 --- a/git-repository/src/repository/config/transport.rs +++ b/git-repository/src/repository/config/transport.rs @@ -48,10 +48,7 @@ impl crate::Repository { use git_transport::client::http; use git_transport::client::http::options::ProxyAuthMethod; - use crate::{ - bstr::ByteVec, - config::cache::util::{ApplyLeniency, ApplyLeniencyDefault}, - }; + use crate::{bstr::ByteVec, config::cache::util::ApplyLeniency}; fn try_cow_to_string( v: Cow<'_, BStr>, lenient: bool, @@ -185,24 +182,28 @@ impl crate::Repository { headers }; - if let Some(follow_redirects) = - config.string_filter("http", None, "followRedirects", &mut trusted_only) + let redirects_key = "http.followRedirects"; + opts.follow_redirects = if config + .string_filter_by_key(redirects_key, &mut trusted_only) + .map_or(false, |v| v.as_ref() == "initial") { - opts.follow_redirects = if follow_redirects.as_ref() == "initial" { - http::options::FollowRedirects::Initial - } else if git_config::Boolean::try_from(follow_redirects) - .map_err(|err| crate::config::transport::Error::ConfigValue { + http::options::FollowRedirects::Initial + } else if let Some(val) = config + .boolean_filter_by_key(redirects_key, &mut trusted_only) + .map(|res| { + res.map_err(|err| crate::config::transport::Error::ConfigValue { source: err, - key: "http.followRedirects", + key: redirects_key, }) - .with_lenient_default(lenient)? - .0 - { - http::options::FollowRedirects::All - } else { - http::options::FollowRedirects::None - }; - } + }) + .transpose() + .with_leniency(lenient)? + { + val.then(|| http::options::FollowRedirects::All) + .unwrap_or(http::options::FollowRedirects::None) + } else { + http::options::FollowRedirects::Initial + }; opts.low_speed_time_seconds = integer(config, lenient, "http.lowSpeedTime", "u64", trusted_only, 0)?; diff --git a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz index 949420ac9ec..118402096ae 100644 --- a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz +++ b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:83f5270e487f0364e4a63f82cf4bfb5cb302bee744e9d3df521b7b3f850f3046 -size 10772 +oid sha256:3687c843a39942197ab8a31896b3a6c453295de7bb8d031642eb34b292a272da +size 10816 diff --git a/git-repository/tests/fixtures/make_config_repos.sh b/git-repository/tests/fixtures/make_config_repos.sh index e7f514eb811..caca1acbd04 100644 --- a/git-repository/tests/fixtures/make_config_repos.sh +++ b/git-repository/tests/fixtures/make_config_repos.sh @@ -6,7 +6,7 @@ git init http-config git config --add http.extraHeader "" git config --add http.extraHeader "ExtraHeader: value2" git config --add http.extraHeader "ExtraHeader: value3" - git config http.followRedirects initial + git config http.followRedirects true git config http.lowSpeedLimit 5k git config http.lowSpeedTime 10 git config http.postBuffer 8k @@ -19,6 +19,8 @@ git init http-config git clone --shared http-config http-remote-override (cd http-remote-override + git config http.followRedirects initial + git config http.proxy http://localhost:9090 git config http.proxyAuthMethod basic @@ -28,6 +30,8 @@ git clone --shared http-config http-remote-override git init http-proxy-empty (cd http-proxy-empty + git config http.followRedirects false + git config http.proxy localhost:9090 git config --add http.proxy "" # a value override disabling it later ) @@ -40,4 +44,8 @@ git init http-proxy-auto-prefix git init http-proxy-authenticated (cd http-proxy-authenticated git config http.proxy user@localhost:9090 + cat <> .git/config +[http] + followRedirects +EOF ) diff --git a/git-repository/tests/repository/config/transport_options.rs b/git-repository/tests/repository/config/transport_options.rs index b551983535e..69c0541f3fe 100644 --- a/git-repository/tests/repository/config/transport_options.rs +++ b/git-repository/tests/repository/config/transport_options.rs @@ -27,11 +27,13 @@ mod http { let git_transport::client::http::Options { proxy, proxy_auth_method, + follow_redirects, .. } = http_options(&repo, Some("origin")); assert_eq!(proxy_auth_method, ProxyAuthMethod::Negotiate); assert_eq!(proxy.as_deref(), Some("http://overridden")); + assert_eq!(follow_redirects, FollowRedirects::Initial); } #[test] @@ -54,7 +56,7 @@ mod http { &["ExtraHeader: value2", "ExtraHeader: value3"], "it respects empty values to clear prior values" ); - assert_eq!(follow_redirects, FollowRedirects::Initial); + assert_eq!(follow_redirects, FollowRedirects::All); assert_eq!(low_speed_limit_bytes_per_second, 5120); assert_eq!(low_speed_time_seconds, 10); assert_eq!(proxy.as_deref(), Some("http://localhost:9090"),); @@ -84,7 +86,12 @@ mod http { assert!( opts.proxy_authenticate.is_some(), "…and credential-helpers are used to do that. This could be overridden in remotes one day" - ) + ); + assert_eq!( + opts.follow_redirects, + FollowRedirects::All, + "an empty value is true, so we can't take shortcuts for these" + ); } #[test] @@ -97,6 +104,7 @@ mod http { Some(""), "empty strings indicate that the proxy is to be unset by the transport" ); + assert_eq!(opts.follow_redirects, FollowRedirects::None); } #[test] @@ -105,5 +113,6 @@ mod http { let opts = http_options(&repo, None); assert_eq!(opts.proxy.as_deref(), Some("http://localhost:9090")); + assert_eq!(opts.follow_redirects, FollowRedirects::Initial); } } From aeb4a1d5cb76316058c7d687e26f5c7db351c09c Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Fri, 25 Nov 2022 17:53:32 +0100 Subject: [PATCH 13/29] feat: add `--strict` option to enforce strict checking of configuration. --- src/plumbing/main.rs | 2 +- src/plumbing/options/mod.rs | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 24633d306c2..8b3ab894052 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -71,7 +71,7 @@ pub fn main() -> Result<()> { let config = config.clone(); move |mode: Mode| -> Result { let mut mapping: git::sec::trust::Mapping = Default::default(); - let strict_toggle = matches!(mode, Mode::Strict | Mode::StrictWithGitInstallConfig); + let strict_toggle = matches!(mode, Mode::Strict | Mode::StrictWithGitInstallConfig) || args.strict; mapping.full = mapping.full.strict_config(strict_toggle); mapping.reduced = mapping.reduced.strict_config(strict_toggle); let git_installation = matches!( diff --git a/src/plumbing/options/mod.rs b/src/plumbing/options/mod.rs index c7cfc1b0509..6dced32578e 100644 --- a/src/plumbing/options/mod.rs +++ b/src/plumbing/options/mod.rs @@ -35,6 +35,14 @@ pub struct Args { #[clap(long, conflicts_with("verbose"))] pub progress: bool, + /// Don't default malformed configuration flags, but show an error instead. + /// + /// Note that some subcommands use strict mode by default. + // TODO: needs a 'lenient' mutually exclusive counterpart. Opens the gate to auto-verbose some commands, and add --no-verbose + // for these. + #[clap(long, short = 's')] + pub strict: bool, + /// The progress TUI will stay up even though the work is already completed. /// /// Use this to be able to read progress messages or additional information visible in the TUI log pane. From 872dc1ab43ce626b4166dae3dc8bddf8e85c9409 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 26 Nov 2022 07:07:37 +0100 Subject: [PATCH 14/29] update progress of http.proxyAuthMethod --- src/plumbing/progress.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index f2f1b1e8eec..cfa68cd997e 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -560,7 +560,7 @@ static GIT_CONFIG: &[Record] = &[ }, Record { config: "http.proxyAuthMethod", - usage: Planned { note: None }, + usage: InModule { name: "repository::config::transport", deviation: Some("implemented like git, but I never tried it so who knows") }, }, Record { config: "http.proxySSLCert", From e4bf8f0072e60a7a2df94690c8d0b13b1f3038bb Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 26 Nov 2022 07:29:54 +0100 Subject: [PATCH 15/29] feat: Add the `Source::EnvOverride` to have a place for 'terminal' overrides. That way environment variables represented via git-configuration can be integrated into git configuration, making clearer what's going to happen even when looking at the configuration via `gix config`. The implementation has to be careful though about assureing there is no more specific configuration key, like `http..proxy` that would override the one from the environment, which always has the final word. --- git-config/src/source.rs | 4 ++-- git-config/src/types.rs | 13 ++++++++++--- 2 files changed, 12 insertions(+), 5 deletions(-) diff --git a/git-config/src/source.rs b/git-config/src/source.rs index c12eb8e8561..6f5add71988 100644 --- a/git-config/src/source.rs +++ b/git-config/src/source.rs @@ -48,7 +48,7 @@ impl Source { System => Kind::System, Git | User => Kind::Global, Local | Worktree => Kind::Repository, - Env | Cli | Api => Kind::Override, + Env | Cli | Api | EnvOverride => Kind::Override, } } @@ -100,7 +100,7 @@ impl Source { }), Local => Some(Path::new("config").into()), Worktree => Some(Path::new("config.worktree").into()), - Env | Cli | Api => None, + Env | Cli | Api | EnvOverride => None, } } } diff --git a/git-config/src/types.rs b/git-config/src/types.rs index f5a75653159..c476c20937f 100644 --- a/git-config/src/types.rs +++ b/git-config/src/types.rs @@ -32,12 +32,19 @@ pub enum Source { /// typically located in `$GIT_DIR/config.worktree` if `extensions.worktreeConfig` /// is enabled. Worktree, - /// values parsed from the environment. + /// Values parsed from the environment using `GIT_CONFIG_COUNT`, + /// `GIT_CONFIG_KEY_N` and `GIT_CONFIG_VALUE_N` where `N` is incremented from 0 up to the + /// value of `GIT_CONFIG_COUNT`. Env, - /// Values set from the command-line. + /// Values set from the command-line, typically controlled by the user running a program. Cli, - /// Entirely internal from a programmatic source + /// Entirely internal from a programmatic source, and can be used to have (near final) say in configuration values. Api, + /// Values obtained from specific environment variables that override values in the git configuration. + /// + /// For example, `HTTP_PROXY` overrides `http.proxy`, no matter where it is specified, and thus + /// controls the value similar to how it's done in `git`. + EnvOverride, } /// High level `git-config` reader and writer. From 5b9bffe8a5eec738e892224a7e18f98c8430d8a4 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 26 Nov 2022 13:49:58 +0100 Subject: [PATCH 16/29] feat: `SectionMut::push_with_comment(key, comment)` to add a new variable with a comment. This is useful for providing more information about a value at hand, especially if it was added programmatically and then shows up in the configuration. --- git-config/src/file/mutable/section.rs | 34 +++++++++++++++++++++++- git-config/tests/file/mutable/section.rs | 31 +++++++++++++++++++++ 2 files changed, 64 insertions(+), 1 deletion(-) diff --git a/git-config/src/file/mutable/section.rs b/git-config/src/file/mutable/section.rs index 225777e08b9..c3570ee3256 100644 --- a/git-config/src/file/mutable/section.rs +++ b/git-config/src/file/mutable/section.rs @@ -28,9 +28,26 @@ pub struct SectionMut<'a, 'event> { /// Mutating methods. impl<'a, 'event> SectionMut<'a, 'event> { - /// Adds an entry to the end of this section name `key` and `value`. If `value` is None`, no equal sign will be written leaving + /// Adds an entry to the end of this section name `key` and `value`. If `value` is `None`, no equal sign will be written leaving /// just the key. This is useful for boolean values which are true if merely the key exists. pub fn push<'b>(&mut self, key: Key<'event>, value: Option<&'b BStr>) { + self.push_with_comment_inner(key, value, None) + } + + /// Adds an entry to the end of this section name `key` and `value`. If `value` is `None`, no equal sign will be written leaving + /// just the key. This is useful for boolean values which are true if merely the key exists. + /// `comment` has to be the text to put right after the value and behind a `#` character. Note that newlines are silently transformed + /// into spaces. + pub fn push_with_comment<'b, 'c>( + &mut self, + key: Key<'event>, + value: Option<&'b BStr>, + comment: impl Into<&'c BStr>, + ) { + self.push_with_comment_inner(key, value, comment.into().into()) + } + + fn push_with_comment_inner(&mut self, key: Key<'event>, value: Option<&BStr>, comment: Option<&BStr>) { let body = &mut self.section.body.0; if let Some(ws) = &self.whitespace.pre_key { body.push(Event::Whitespace(ws.clone())); @@ -44,6 +61,21 @@ impl<'a, 'event> SectionMut<'a, 'event> { } None => body.push(Event::Value(Cow::Borrowed("".into()))), } + if let Some(comment) = comment { + body.push(Event::Whitespace(Cow::Borrowed(" ".into()))); + body.push(Event::Comment(parse::Comment { + tag: b'#', + text: Cow::Owned({ + let mut c = Vec::with_capacity(comment.len()); + let mut bytes = comment.iter().peekable(); + if !bytes.peek().map_or(true, |b| b.is_ascii_whitespace()) { + c.insert(0, b' '); + } + c.extend(bytes.map(|b| (*b == b'\n').then(|| b' ').unwrap_or(*b))); + c.into() + }), + })); + } if self.implicit_newline { body.push(Event::Newline(BString::from(self.newline.to_vec()).into())); } diff --git a/git-config/tests/file/mutable/section.rs b/git-config/tests/file/mutable/section.rs index 83e13e69166..a22644c9a9b 100644 --- a/git-config/tests/file/mutable/section.rs +++ b/git-config/tests/file/mutable/section.rs @@ -202,6 +202,37 @@ mod push { } } +mod push_with_comment { + use git_config::parse::section::Key; + + #[test] + fn various_comments_and_escaping() { + for (comment, expected) in [ + ("", "$head\tk = v #$nl"), + ("this is v!", "$head\tk = v # this is v!$nl"), + (" no double space", "$head\tk = v # no double space$nl"), + ("\tno double whitespace", "$head\tk = v #\tno double whitespace$nl"), + ( + "one\ntwo\nnewlines are replaced with space", + "$head\tk = v # one two newlines are replaced with space$nl", + ), + ( + "a\rb\r\nlinefeeds aren't special", + "$head\tk = v # a\rb\r linefeeds aren't special$nl", + ), + ] { + let mut config = git_config::File::default(); + let mut section = config.new_section("a", None).unwrap(); + section.set_implicit_newline(false); + section.push_with_comment(Key::try_from("k").unwrap(), Some("v".into()), comment); + let expected = expected + .replace("$head", &format!("[a]{nl}", nl = section.newline())) + .replace("$nl", §ion.newline().to_string()); + assert_eq!(config.to_bstring(), expected); + } + } +} + mod set_leading_whitespace { use std::{borrow::Cow, convert::TryFrom}; From 2b36d99eaf3ed24ce4cb736a3dd48440dc0c73b7 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 10:43:41 +0100 Subject: [PATCH 17/29] feat!: `File::new_section()` and related now returns their `id` as well. That way it's possible to more easily interact with it later, for instance when one wants to delete it. --- git-config/src/file/init/mod.rs | 1 + git-config/src/file/mod.rs | 7 +++++++ git-config/src/file/section/mod.rs | 8 ++++++++ git-config/src/file/util.rs | 6 ++++-- 4 files changed, 20 insertions(+), 2 deletions(-) diff --git a/git-config/src/file/init/mod.rs b/git-config/src/file/init/mod.rs index d44dece91cd..f2a4f91e898 100644 --- a/git-config/src/file/init/mod.rs +++ b/git-config/src/file/init/mod.rs @@ -58,6 +58,7 @@ impl<'a> File<'a> { header: section.header, body: section::Body(section.events), meta: OwnShared::clone(&meta), + id: Default::default(), }); } diff --git a/git-config/src/file/mod.rs b/git-config/src/file/mod.rs index 16209772ead..e86e03a0739 100644 --- a/git-config/src/file/mod.rs +++ b/git-config/src/file/mod.rs @@ -72,6 +72,7 @@ pub struct Section<'a> { header: crate::parse::section::Header<'a>, body: section::Body<'a>, meta: OwnShared, + id: SectionId, } /// A function to filter metadata, returning `true` if the corresponding but omitted value can be used. @@ -112,6 +113,12 @@ impl AddAssign for Size { #[derive(PartialEq, Eq, Hash, Copy, Clone, PartialOrd, Ord, Debug)] pub struct SectionId(pub(crate) usize); +impl Default for SectionId { + fn default() -> Self { + SectionId(usize::MAX) + } +} + /// All section body ids referred to by a section name. /// /// Note that order in Vec matters as it represents the order diff --git a/git-config/src/file/section/mod.rs b/git-config/src/file/section/mod.rs index 392686b7bc4..41f526d6c53 100644 --- a/git-config/src/file/section/mod.rs +++ b/git-config/src/file/section/mod.rs @@ -11,6 +11,7 @@ use crate::{ }; pub(crate) mod body; +use crate::file::SectionId; pub use body::{Body, BodyIter}; use git_features::threading::OwnShared; @@ -36,6 +37,7 @@ impl<'a> Section<'a> { header: parse::section::Header::new(name, subsection)?, body: Default::default(), meta: meta.into(), + id: SectionId::default(), }) } } @@ -47,6 +49,12 @@ impl<'a> Section<'a> { &self.header } + /// Return the unique `id` of the section, for use with the `*_by_id()` family of methods + /// in [git_config::File][crate::File]. + pub fn id(&self) -> SectionId { + self.id + } + /// Return our body, containing all keys and values. pub fn body(&self) -> &Body<'a> { &self.body diff --git a/git-config/src/file/util.rs b/git-config/src/file/util.rs index ac8424e923a..f00ccc31e57 100644 --- a/git-config/src/file/util.rs +++ b/git-config/src/file/util.rs @@ -12,8 +12,9 @@ use crate::{ /// Private helper functions impl<'event> File<'event> { /// Adds a new section to the config file, returning the section id of the newly added section. - pub(crate) fn push_section_internal(&mut self, section: file::Section<'event>) -> SectionId { + pub(crate) fn push_section_internal(&mut self, mut section: file::Section<'event>) -> SectionId { let new_section_id = SectionId(self.section_id_counter); + section.id = new_section_id; self.sections.insert(new_section_id, section); let header = &self.sections[&new_section_id].header; let lookup = self.section_lookup_tree.entry(header.name.clone()).or_default(); @@ -53,7 +54,7 @@ impl<'event> File<'event> { } /// Inserts `section` after the section that comes `before` it, and maintains correct ordering in all of our lookup structures. - pub(crate) fn insert_section_after(&mut self, section: file::Section<'event>, before: SectionId) -> SectionId { + pub(crate) fn insert_section_after(&mut self, mut section: file::Section<'event>, before: SectionId) -> SectionId { let lookup_section_order = { let section_order = &self.section_order; move |section_id| { @@ -67,6 +68,7 @@ impl<'event> File<'event> { let before_order = lookup_section_order(before); let new_section_id = SectionId(self.section_id_counter); + section.id = new_section_id; self.sections.insert(new_section_id, section); let header = &self.sections[&new_section_id].header; let lookup = self.section_lookup_tree.entry(header.name.clone()).or_default(); From f16e36168cc93768ba5d53c9848ff2e8432d06b1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 28 Nov 2022 15:48:19 +0100 Subject: [PATCH 18/29] feat!: remove `SnapshotMut::apply_cli_overrides()` in favor of `open::Options::cli_overrides()`. --- git-repository/src/clone/fetch/util.rs | 2 +- git-repository/src/config/cache/init.rs | 10 +++++-- git-repository/src/config/overrides.rs | 2 +- git-repository/src/config/snapshot/access.rs | 12 +++++--- git-repository/src/open.rs | 30 ++++++++++++++----- .../repository/config/config_snapshot/mod.rs | 15 ++++++---- 6 files changed, 48 insertions(+), 23 deletions(-) diff --git a/git-repository/src/clone/fetch/util.rs b/git-repository/src/clone/fetch/util.rs index 22aa26fa674..295c1a64267 100644 --- a/git-repository/src/clone/fetch/util.rs +++ b/git-repository/src/clone/fetch/util.rs @@ -38,7 +38,7 @@ pub fn replace_changed_local_config_file(repo: &mut Repository, mut config: git_ for id in ids_to_remove { repo_config.remove_section_by_id(id); } - crate::config::overrides::apply(&mut config, &repo.options.config_overrides, git_config::Source::Api) + crate::config::overrides::append(&mut config, &repo.options.api_config_overrides, git_config::Source::Api) .expect("applied once and can be applied again"); repo_config.append(config); repo.reread_values_and_clear_caches() diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index 7f69b833d28..baa226f5601 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -37,7 +37,8 @@ impl Cache { includes: use_includes, }: repository::permissions::Config, lenient_config: bool, - config_overrides: &[BString], + api_config_overrides: &[BString], + cli_config_overrides: &[BString], ) -> Result { let options = git_config::file::init::Options { includes: if use_includes { @@ -113,8 +114,11 @@ impl Cache { if use_env { globals.append(git_config::File::from_env(options)?.unwrap_or_default()); } - if !config_overrides.is_empty() { - crate::config::overrides::apply(&mut globals, config_overrides, git_config::Source::Api)?; + if !cli_config_overrides.is_empty() { + crate::config::overrides::append(&mut globals, cli_config_overrides, git_config::Source::Cli)?; + } + if !api_config_overrides.is_empty() { + crate::config::overrides::append(&mut globals, api_config_overrides, git_config::Source::Api)?; } globals }; diff --git a/git-repository/src/config/overrides.rs b/git-repository/src/config/overrides.rs index a29466aa848..57d23386a8b 100644 --- a/git-repository/src/config/overrides.rs +++ b/git-repository/src/config/overrides.rs @@ -17,7 +17,7 @@ pub enum Error { SectionHeader(#[from] git_config::parse::section::header::Error), } -pub(crate) fn apply( +pub(crate) fn append( config: &mut git_config::File<'static>, values: impl IntoIterator>, source: git_config::Source, diff --git a/git-repository/src/config/snapshot/access.rs b/git-repository/src/config/snapshot/access.rs index fa66ba77256..a1abeb43aaf 100644 --- a/git-repository/src/config/snapshot/access.rs +++ b/git-repository/src/config/snapshot/access.rs @@ -88,13 +88,17 @@ impl<'repo> Snapshot<'repo> { /// Utilities impl<'repo> SnapshotMut<'repo> { - /// Apply configuration values of the form `core.abbrev=5` or `remote.origin.url = foo` or `core.bool-implicit-true` - /// to the repository configuration, marked with [source CLI][git_config::Source::Cli]. - pub fn apply_cli_overrides( + /// Append configuration values of the form `core.abbrev=5` or `remote.origin.url = foo` or `core.bool-implicit-true` + /// to the end of the repository configuration, with each section marked with the given `source`. + /// + /// Note that doing so applies the configuration at the very end, so it will always override what came before it + /// even though the `source` is of lower priority as what's there. + pub fn append_config( &mut self, values: impl IntoIterator>, + source: git_config::Source, ) -> Result<&mut Self, crate::config::overrides::Error> { - crate::config::overrides::apply(&mut self.config, values, git_config::Source::Cli)?; + crate::config::overrides::append(&mut self.config, values, source)?; Ok(self) } /// Apply all changes made to this instance. diff --git a/git-repository/src/open.rs b/git-repository/src/open.rs index 779ecff6c22..e499355b1a3 100644 --- a/git-repository/src/open.rs +++ b/git-repository/src/open.rs @@ -75,7 +75,8 @@ pub struct Options { pub(crate) lossy_config: Option, pub(crate) lenient_config: bool, pub(crate) bail_if_untrusted: bool, - pub(crate) config_overrides: Vec, + pub(crate) api_config_overrides: Vec, + pub(crate) cli_config_overrides: Vec, /// Internal to pass an already obtained CWD on to where it may also be used. This avoids the CWD being queried more than once per repo. pub(crate) current_dir: Option, } @@ -91,7 +92,8 @@ impl Default for Options { lossy_config: None, lenient_config: true, bail_if_untrusted: false, - config_overrides: Vec::new(), + api_config_overrides: Vec::new(), + cli_config_overrides: Vec::new(), current_dir: None, } } @@ -138,7 +140,15 @@ impl Options { /// as the configuration is initialized to allow affecting the repository instantiation phase, both on disk or when opening. /// The configuration is marked with [source API][git_config::Source::Api]. pub fn config_overrides(mut self, values: impl IntoIterator>) -> Self { - self.config_overrides = values.into_iter().map(Into::into).collect(); + self.api_config_overrides = values.into_iter().map(Into::into).collect(); + self + } + + /// Set configuration values of the form `core.abbrev=5` or `remote.origin.url = foo` or `core.bool-implicit-true` for application + /// as CLI overrides to the repository configuration, marked with [source CLI][git_config::Source::Cli]. + /// These are equivalent to CLI overrides passed with `-c` in `git`, for example. + pub fn cli_overrides(mut self, values: impl IntoIterator>) -> Self { + self.cli_config_overrides = values.into_iter().map(Into::into).collect(); self } @@ -241,7 +251,8 @@ impl git_sec::trust::DefaultForLevel for Options { lossy_config: None, bail_if_untrusted: false, lenient_config: true, - config_overrides: Vec::new(), + api_config_overrides: Vec::new(), + cli_config_overrides: Vec::new(), current_dir: None, }, git_sec::Trust::Reduced => Options { @@ -253,7 +264,8 @@ impl git_sec::trust::DefaultForLevel for Options { bail_if_untrusted: false, lenient_config: true, lossy_config: None, - config_overrides: Vec::new(), + api_config_overrides: Vec::new(), + cli_config_overrides: Vec::new(), current_dir: None, }, } @@ -361,7 +373,8 @@ impl ThreadSafeRepository { lenient_config, bail_if_untrusted, permissions: Permissions { ref env, config }, - ref config_overrides, + ref api_config_overrides, + ref cli_config_overrides, ref current_dir, } = options; let current_dir = current_dir.as_deref().expect("BUG: current_dir must be set by caller"); @@ -402,7 +415,8 @@ impl ThreadSafeRepository { env.clone(), config, lenient_config, - config_overrides, + api_config_overrides, + cli_config_overrides, )?; if bail_if_untrusted && git_dir_trust != git_sec::Trust::Full { @@ -571,7 +585,7 @@ mod tests { #[test] fn size_of_options() { let actual = std::mem::size_of::(); - let limit = 140; + let limit = 160; assert!( actual <= limit, "{} <= {}: size shouldn't change without us knowing (on windows, it's bigger)", diff --git a/git-repository/tests/repository/config/config_snapshot/mod.rs b/git-repository/tests/repository/config/config_snapshot/mod.rs index 20884cd4814..9803cd58ae4 100644 --- a/git-repository/tests/repository/config/config_snapshot/mod.rs +++ b/git-repository/tests/repository/config/config_snapshot/mod.rs @@ -84,12 +84,15 @@ fn values_are_set_in_memory_only() { #[test] fn apply_cli_overrides() -> crate::Result { let mut repo = named_repo("make_config_repo.sh").unwrap(); - repo.config_snapshot_mut().apply_cli_overrides([ - "a.b=c", - "remote.origin.url = url", - "implicit.bool-true", - "implicit.bool-false = ", - ])?; + repo.config_snapshot_mut().append_config( + [ + "a.b=c", + "remote.origin.url = url", + "implicit.bool-true", + "implicit.bool-false = ", + ], + git_config::Source::Cli, + )?; let config = repo.config_snapshot(); assert_eq!(config.string("a.b").expect("present").as_ref(), "c"); From 0ce29a965cf16273cf74bd22e40f464e322e2f62 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 28 Nov 2022 19:00:43 +0100 Subject: [PATCH 19/29] feat: `open::Options::modify()` as general pattern to allow builder methods usage in `&mut self`. That way it's easier to configure both the `full` and the `partial` trust instances of discovery options. --- git-repository/src/config/cache/init.rs | 139 +++++++++++++----------- git-repository/src/config/mod.rs | 8 +- git-repository/src/open.rs | 8 ++ 3 files changed, 88 insertions(+), 67 deletions(-) diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index baa226f5601..06b5bd52097 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -55,73 +55,82 @@ impl Cache { ..util::base_options(lossy) }; - let config = { - let home_env = &home_env; - let xdg_config_home_env = &xdg_config_home_env; - let git_prefix = &git_prefix; - let metas = [ - git_config::source::Kind::GitInstallation, - git_config::source::Kind::System, - git_config::source::Kind::Global, - ] - .iter() - .flat_map(|kind| kind.sources()) - .filter_map(|source| { - match source { - git_config::Source::GitInstallation if !use_installation => return None, - git_config::Source::System if !use_system => return None, - git_config::Source::Git if !use_git => return None, - git_config::Source::User if !use_user => return None, - _ => {} - } - source - .storage_location(&mut |name| { - match name { - git_ if git_.starts_with("GIT_") => Some(git_prefix), - "XDG_CONFIG_HOME" => Some(xdg_config_home_env), - "HOME" => Some(home_env), - _ => None, - } - .and_then(|perm| std::env::var_os(name).and_then(|val| perm.check_opt(val))) - }) - .map(|p| (source, p.into_owned())) - }) - .map(|(source, path)| git_config::file::Metadata { - path: Some(path), - source: *source, - level: 0, - trust: git_sec::Trust::Full, - }); + let config = + { + let home_env = &home_env; + let xdg_config_home_env = &xdg_config_home_env; + let git_prefix = &git_prefix; + let metas = [ + git_config::source::Kind::GitInstallation, + git_config::source::Kind::System, + git_config::source::Kind::Global, + ] + .iter() + .flat_map(|kind| kind.sources()) + .filter_map(|source| { + match source { + git_config::Source::GitInstallation if !use_installation => return None, + git_config::Source::System if !use_system => return None, + git_config::Source::Git if !use_git => return None, + git_config::Source::User if !use_user => return None, + _ => {} + } + source + .storage_location(&mut |name| { + match name { + git_ if git_.starts_with("GIT_") => Some(git_prefix), + "XDG_CONFIG_HOME" => Some(xdg_config_home_env), + "HOME" => Some(home_env), + _ => None, + } + .and_then(|perm| std::env::var_os(name).and_then(|val| perm.check_opt(val))) + }) + .map(|p| (source, p.into_owned())) + }) + .map(|(source, path)| git_config::file::Metadata { + path: Some(path), + source: *source, + level: 0, + trust: git_sec::Trust::Full, + }); - let err_on_nonexisting_paths = false; - let mut globals = git_config::File::from_paths_metadata_buf( - metas, - &mut buf, - err_on_nonexisting_paths, - git_config::file::init::Options { - includes: git_config::file::includes::Options::no_follow(), - ..options - }, - ) - .map_err(|err| match err { - git_config::file::init::from_paths::Error::Init(err) => Error::from(err), - git_config::file::init::from_paths::Error::Io(err) => err.into(), - })? - .unwrap_or_default(); + let err_on_nonexisting_paths = false; + let mut globals = git_config::File::from_paths_metadata_buf( + metas, + &mut buf, + err_on_nonexisting_paths, + git_config::file::init::Options { + includes: git_config::file::includes::Options::no_follow(), + ..options + }, + ) + .map_err(|err| match err { + git_config::file::init::from_paths::Error::Init(err) => Error::from(err), + git_config::file::init::from_paths::Error::Io(err) => err.into(), + })? + .unwrap_or_default(); - globals.append(git_dir_config); - globals.resolve_includes(options)?; - if use_env { - globals.append(git_config::File::from_env(options)?.unwrap_or_default()); - } - if !cli_config_overrides.is_empty() { - crate::config::overrides::append(&mut globals, cli_config_overrides, git_config::Source::Cli)?; - } - if !api_config_overrides.is_empty() { - crate::config::overrides::append(&mut globals, api_config_overrides, git_config::Source::Api)?; - } - globals - }; + globals.append(git_dir_config); + globals.resolve_includes(options)?; + if use_env { + globals.append(git_config::File::from_env(options)?.unwrap_or_default()); + } + if !cli_config_overrides.is_empty() { + crate::config::overrides::append(&mut globals, cli_config_overrides, git_config::Source::Cli) + .map_err(|err| Error::ConfigOverrides { + err, + source: git_config::Source::Cli, + })?; + } + if !api_config_overrides.is_empty() { + crate::config::overrides::append(&mut globals, api_config_overrides, git_config::Source::Api) + .map_err(|err| Error::ConfigOverrides { + err, + source: git_config::Source::Api, + })?; + } + globals + }; let hex_len = util::parse_core_abbrev(&config, object_hash).with_leniency(lenient_config)?; diff --git a/git-repository/src/config/mod.rs b/git-repository/src/config/mod.rs index 88ffb6970f4..4e6d780e054 100644 --- a/git-repository/src/config/mod.rs +++ b/git-repository/src/config/mod.rs @@ -67,8 +67,12 @@ pub enum Error { DecodeBoolean { key: String, value: BString }, #[error(transparent)] PathInterpolation(#[from] git_config::path::interpolate::Error), - #[error("Configuration overrides at open or init time could not be applied.")] - ConfigOverrides(#[from] overrides::Error), + #[error("{source:?} configuration overrides at open or init time could not be applied.")] + ConfigOverrides { + #[source] + err: overrides::Error, + source: git_config::Source, + }, #[error("Invalid value for 'core.logAllRefUpdates': \"{value}\"")] LogAllRefUpdates { value: BString }, } diff --git a/git-repository/src/open.rs b/git-repository/src/open.rs index e499355b1a3..050c505fdce 100644 --- a/git-repository/src/open.rs +++ b/git-repository/src/open.rs @@ -134,6 +134,14 @@ impl Options { } } +/// Generic modification +impl Options { + /// An adapter to allow calling any builder method on this instance despite only having a mutable reference. + pub fn modify(&mut self, f: impl FnOnce(Self) -> Self) { + *self = f(std::mem::take(self)); + } +} + /// Builder methods impl Options { /// Apply the given configuration `values` like `init.defaultBranch=special` or `core.bool-implicit-true` in memory to as early From f1a4c8b42ed8c94e7fe3a61eb222cf6b0886f4ee Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 28 Nov 2022 16:19:44 +0100 Subject: [PATCH 20/29] adapt to changes in `git-repository` --- gitoxide-core/src/repository/config.rs | 6 ++++-- src/plumbing/main.rs | 14 +++++++++++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/gitoxide-core/src/repository/config.rs b/gitoxide-core/src/repository/config.rs index 11bfb43fe42..82d033c6789 100644 --- a/gitoxide-core/src/repository/config.rs +++ b/gitoxide-core/src/repository/config.rs @@ -14,8 +14,10 @@ pub fn list( if format != OutputFormat::Human { bail!("Only human output format is supported at the moment"); } - let mut repo = git::open_opts(repo.git_dir(), repo.open_options().clone().lossy_config(false))?; - repo.config_snapshot_mut().apply_cli_overrides(overrides.into_iter())?; + let repo = git::open_opts( + repo.git_dir(), + repo.open_options().clone().lossy_config(false).cli_overrides(overrides), + )?; let config = repo.config_snapshot(); let config = config.plumbing(); if let Some(frontmatter) = config.frontmatter() { diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 8b3ab894052..49b21e84e48 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -78,14 +78,22 @@ pub fn main() -> Result<()> { mode, Mode::StrictWithGitInstallConfig | Mode::LenientWithGitInstallConfig ); - mapping.full.permissions.config.git_binary = git_installation; - mapping.reduced.permissions.config.git_binary = git_installation; + let to_match_settings = |mut opts: git::open::Options| { + opts.permissions.config.git_binary = git_installation; + if config.is_empty() { + opts + } else { + opts.cli_overrides(config.clone()) + } + }; + mapping.full.modify(to_match_settings); + mapping.reduced.modify(to_match_settings); let mut repo = git::ThreadSafeRepository::discover_opts(repository, Default::default(), mapping) .map(git::Repository::from) .map(|r| r.apply_environment())?; if !config.is_empty() { repo.config_snapshot_mut() - .apply_cli_overrides(config.iter()) + .append_config(config.iter(), git::config::Source::Cli) .context("Unable to parse command-line configuration")?; } Ok(repo) From 603f341e71c021bcc0f154c2ce6c39f4e6546c12 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Mon, 28 Nov 2022 21:13:21 +0100 Subject: [PATCH 21/29] refactor - use improved API in git-config - expose `plumbing` directly via AsDeref. --- git-repository/src/config/snapshot/_impls.rs | 8 ++++++++ git-repository/src/config/snapshot/access.rs | 18 +++--------------- gitoxide-core/src/repository/config.rs | 1 - 3 files changed, 11 insertions(+), 16 deletions(-) diff --git a/git-repository/src/config/snapshot/_impls.rs b/git-repository/src/config/snapshot/_impls.rs index 7ebd6835724..7d7e98caed8 100644 --- a/git-repository/src/config/snapshot/_impls.rs +++ b/git-repository/src/config/snapshot/_impls.rs @@ -47,6 +47,14 @@ impl Deref for SnapshotMut<'_> { } } +impl Deref for Snapshot<'_> { + type Target = git_config::File<'static>; + + fn deref(&self) -> &Self::Target { + &self.plumbing() + } +} + impl Deref for CommitAutoRollback<'_> { type Target = crate::Repository; diff --git a/git-repository/src/config/snapshot/access.rs b/git-repository/src/config/snapshot/access.rs index a1abeb43aaf..1232cba6c2f 100644 --- a/git-repository/src/config/snapshot/access.rs +++ b/git-repository/src/config/snapshot/access.rs @@ -25,11 +25,7 @@ impl<'repo> Snapshot<'repo> { /// Like [`boolean()`][Self::boolean()], but it will report an error if the value couldn't be interpreted as boolean. pub fn try_boolean<'a>(&self, key: impl Into<&'a BStr>) -> Option> { - let key = git_config::parse::key(key)?; - self.repo - .config - .resolved - .boolean(key.section_name, key.subsection_name, key.value_name) + self.repo.config.resolved.boolean_by_key(key) } /// Return the resolved integer at `key`, or `None` if there is no such value or if the value can't be interpreted as @@ -44,22 +40,14 @@ impl<'repo> Snapshot<'repo> { /// Like [`integer()`][Self::integer()], but it will report an error if the value couldn't be interpreted as boolean. pub fn try_integer<'a>(&self, key: impl Into<&'a BStr>) -> Option> { - let key = git_config::parse::key(key)?; - self.repo - .config - .resolved - .integer(key.section_name, key.subsection_name, key.value_name) + self.repo.config.resolved.integer_by_key(key) } /// Return the string at `key`, or `None` if there is no such value. /// /// Note that this method takes the most recent value at `key` even if it is from a file with reduced trust. pub fn string<'a>(&self, key: impl Into<&'a BStr>) -> Option> { - let key = git_config::parse::key(key)?; - self.repo - .config - .resolved - .string(key.section_name, key.subsection_name, key.value_name) + self.repo.config.resolved.string_by_key(key) } /// Return the trusted and fully interpolated path at `key`, or `None` if there is no such value diff --git a/gitoxide-core/src/repository/config.rs b/gitoxide-core/src/repository/config.rs index 82d033c6789..7eb58d13217 100644 --- a/gitoxide-core/src/repository/config.rs +++ b/gitoxide-core/src/repository/config.rs @@ -19,7 +19,6 @@ pub fn list( repo.open_options().clone().lossy_config(false).cli_overrides(overrides), )?; let config = repo.config_snapshot(); - let config = config.plumbing(); if let Some(frontmatter) = config.frontmatter() { for event in frontmatter { event.write_to(&mut out)?; From e701e7e9cc571108ca210fc0ca23494d6a1c7208 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 10:11:49 +0100 Subject: [PATCH 22/29] feat: `client::http::Options::verbose` to see more debug output. This means different things depending on the backend, and for `curl` it means a lot of debug-output on stderr. --- git-transport/src/client/blocking_io/http/curl/remote.rs | 2 ++ git-transport/src/client/blocking_io/http/mod.rs | 2 ++ 2 files changed, 4 insertions(+) diff --git a/git-transport/src/client/blocking_io/http/curl/remote.rs b/git-transport/src/client/blocking_io/http/curl/remote.rs index e4c1a5db4a3..d6aa25b1885 100644 --- a/git-transport/src/client/blocking_io/http/curl/remote.rs +++ b/git-transport/src/client/blocking_io/http/curl/remote.rs @@ -140,6 +140,7 @@ pub fn new() -> ( proxy_auth_method, user_agent, proxy_authenticate, + verbose, backend: _, }, } in req_recv @@ -152,6 +153,7 @@ pub fn new() -> ( for header in extra_headers { headers.append(&header)?; } + handle.verbose(verbose)?; let mut proxy_auth_action = None; if let Some(proxy) = proxy { diff --git a/git-transport/src/client/blocking_io/http/mod.rs b/git-transport/src/client/blocking_io/http/mod.rs index da779e51c71..3c5bd027ce0 100644 --- a/git-transport/src/client/blocking_io/http/mod.rs +++ b/git-transport/src/client/blocking_io/http/mod.rs @@ -123,6 +123,8 @@ pub struct Options { /// If `None`, this typically defaults to 2 minutes to 5 minutes. /// Refers to `gitoxide.http.connectTimeout`. pub connect_timeout: Option, + /// If enabled, emit additional information about connections and possibly the data received or written. + pub verbose: bool, /// Backend specific options, if available. pub backend: Option>>, } From 5034544b36994177009ccc8d6c07cb000b429174 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 14:06:36 +0100 Subject: [PATCH 23/29] feat: `client::http::Options::no_proxy` to disable a proxy for given hosts. This is a curl-first option which can reasonably be implemented for other backends as well and thus retains its curl-ish roots in full. --- git-transport/src/client/blocking_io/http/curl/remote.rs | 4 ++++ git-transport/src/client/blocking_io/http/mod.rs | 2 ++ 2 files changed, 6 insertions(+) diff --git a/git-transport/src/client/blocking_io/http/curl/remote.rs b/git-transport/src/client/blocking_io/http/curl/remote.rs index d6aa25b1885..2ea848eceb2 100644 --- a/git-transport/src/client/blocking_io/http/curl/remote.rs +++ b/git-transport/src/client/blocking_io/http/curl/remote.rs @@ -137,6 +137,7 @@ pub fn new() -> ( low_speed_time_seconds, connect_timeout, proxy, + no_proxy, proxy_auth_method, user_agent, proxy_authenticate, @@ -179,6 +180,9 @@ pub fn new() -> ( proxy_auth_action = Some((creds.next, authenticate)); } } + if let Some(no_proxy) = no_proxy { + handle.noproxy(&no_proxy)?; + } if let Some(user_agent) = user_agent { handle.useragent(&user_agent)?; } diff --git a/git-transport/src/client/blocking_io/http/mod.rs b/git-transport/src/client/blocking_io/http/mod.rs index 3c5bd027ce0..a75dcccc50c 100644 --- a/git-transport/src/client/blocking_io/http/mod.rs +++ b/git-transport/src/client/blocking_io/http/mod.rs @@ -101,6 +101,8 @@ pub struct Options { /// /// Refers to `http.proxy`. pub proxy: Option, + /// The comma-separated list of hosts to not send through the `proxy`, or `*` to entirely disable all proxying. + pub no_proxy: Option, /// The way to authenticate against the proxy if the `proxy` field contains a username. /// /// Refers to `http.proxyAuthMethod`. From fc64693d5af0fda402c560d10d15652c24d14219 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 11:02:56 +0100 Subject: [PATCH 24/29] feat: add `permissions::Environment::http_transport`. That way it's possible to deny using environment variables that affect the HTTP transport, like setting the proxy. --- git-repository/src/config/cache/init.rs | 1 + git-repository/src/repository/permissions.rs | 7 +++++++ .../tests/repository/config/transport_options.rs | 2 ++ 3 files changed, 10 insertions(+) diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index 06b5bd52097..90de62bc90b 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -27,6 +27,7 @@ impl Cache { home: home_env, xdg_config_home: xdg_config_home_env, ssh_prefix: _, + http_transport: _, }: repository::permissions::Environment, repository::permissions::Config { git_binary: use_installation, diff --git a/git-repository/src/repository/permissions.rs b/git-repository/src/repository/permissions.rs index 8cfbcf26269..b4ac124eafe 100644 --- a/git-repository/src/repository/permissions.rs +++ b/git-repository/src/repository/permissions.rs @@ -72,6 +72,11 @@ pub struct Environment { pub git_prefix: git_sec::Permission, /// Control if resources pointed to by `SSH_*` prefixed environment variables can be used (like `SSH_ASKPASS`) pub ssh_prefix: git_sec::Permission, + /// Control if environment variables to configure the HTTP transport, like `http_proxy` may be used. + /// + /// Note that those http-transport related environment variables prefixed with `GIT_` are falling under the + /// `git_prefix` permission, like `GIT_HTTP_USER_AGENT`. + pub http_transport: git_sec::Permission, } impl Environment { @@ -82,6 +87,7 @@ impl Environment { home: git_sec::Permission::Allow, git_prefix: git_sec::Permission::Allow, ssh_prefix: git_sec::Permission::Allow, + http_transport: git_sec::Permission::Allow, } } } @@ -126,6 +132,7 @@ impl Permissions { home: deny, ssh_prefix: deny, git_prefix: deny, + http_transport: deny, } }, } diff --git a/git-repository/tests/repository/config/transport_options.rs b/git-repository/tests/repository/config/transport_options.rs index 69c0541f3fe..0bfd5075e21 100644 --- a/git-repository/tests/repository/config/transport_options.rs +++ b/git-repository/tests/repository/config/transport_options.rs @@ -45,10 +45,12 @@ mod http { low_speed_limit_bytes_per_second, low_speed_time_seconds, proxy, + no_proxy: _, proxy_auth_method, proxy_authenticate, user_agent, connect_timeout, + verbose: _, backend, } = http_options(&repo, None); assert_eq!( From 9441c261bcae61d1d1e674b5e783f38b0471be29 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 26 Nov 2022 07:25:36 +0100 Subject: [PATCH 25/29] apply related environment variables as config overrides Using git-configuration to store overrides from the environment is helpful as it more tighly integrates with the best configuration system there is, and also is nicely visualizable. It's now done for transport related configuration as well as all other environment variables we previously queried on the fly. --- git-repository/src/clone/fetch/util.rs | 9 +- git-repository/src/config/cache/access.rs | 7 +- git-repository/src/config/cache/init.rs | 296 ++++++--- git-repository/src/config/cache/util.rs | 11 + git-repository/src/config/overrides.rs | 15 +- git-repository/src/config/snapshot/_impls.rs | 2 +- git-repository/src/config/snapshot/access.rs | 2 +- git-repository/src/open.rs | 604 ------------------ git-repository/src/open/mod.rs | 114 ++++ git-repository/src/open/options.rs | 177 +++++ git-repository/src/open/repository.rs | 281 ++++++++ .../src/remote/url/scheme_permission.rs | 10 +- .../src/repository/config/transport.rs | 59 +- .../make_config_repos.tar.xz | 4 +- .../tests/fixtures/make_config_repos.sh | 50 ++ .../repository/config/transport_options.rs | 146 ++++- git-repository/tests/repository/mod.rs | 2 +- git-repository/tests/repository/open.rs | 127 ++++ git-repository/tests/util/mod.rs | 5 + git-sec/src/permission.rs | 2 +- src/plumbing/progress.rs | 46 +- 21 files changed, 1240 insertions(+), 729 deletions(-) delete mode 100644 git-repository/src/open.rs create mode 100644 git-repository/src/open/mod.rs create mode 100644 git-repository/src/open/options.rs create mode 100644 git-repository/src/open/repository.rs diff --git a/git-repository/src/clone/fetch/util.rs b/git-repository/src/clone/fetch/util.rs index 295c1a64267..3918b29bae4 100644 --- a/git-repository/src/clone/fetch/util.rs +++ b/git-repository/src/clone/fetch/util.rs @@ -38,8 +38,13 @@ pub fn replace_changed_local_config_file(repo: &mut Repository, mut config: git_ for id in ids_to_remove { repo_config.remove_section_by_id(id); } - crate::config::overrides::append(&mut config, &repo.options.api_config_overrides, git_config::Source::Api) - .expect("applied once and can be applied again"); + crate::config::overrides::append( + &mut config, + &repo.options.api_config_overrides, + git_config::Source::Api, + |_| None, + ) + .expect("applied once and can be applied again"); repo_config.append(config); repo.reread_values_and_clear_caches() .expect("values could be read once and can be read again"); diff --git a/git-repository/src/config/cache/access.rs b/git-repository/src/config/cache/access.rs index dce0168d958..41017e20233 100644 --- a/git-repository/src/config/cache/access.rs +++ b/git-repository/src/config/cache/access.rs @@ -50,7 +50,7 @@ impl Cache { .user_agent .get_or_init(|| { self.resolved - .string("gitoxide", None, "userAgent") + .string_by_key("gitoxide.userAgent") .map(|s| s.to_string()) .unwrap_or_else(|| crate::env::agent().into()) }) @@ -72,9 +72,8 @@ impl Cache { pub(crate) fn url_scheme( &self, ) -> Result<&remote::url::SchemePermission, remote::url::scheme_permission::init::Error> { - self.url_scheme.get_or_try_init(|| { - remote::url::SchemePermission::from_config(&self.resolved, self.git_prefix, self.filter_config_section) - }) + self.url_scheme + .get_or_try_init(|| remote::url::SchemePermission::from_config(&self.resolved, self.filter_config_section)) } /// Returns (file-timeout, pack-refs timeout) diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index 90de62bc90b..aa34c4c2a05 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -4,6 +4,9 @@ use crate::{ config::{cache::util::ApplyLeniency, Cache}, repository, }; +use git_config::File; +use git_sec::Permission; +use std::borrow::Cow; /// Initialization impl Cache { @@ -27,7 +30,7 @@ impl Cache { home: home_env, xdg_config_home: xdg_config_home_env, ssh_prefix: _, - http_transport: _, + http_transport, }: repository::permissions::Environment, repository::permissions::Config { git_binary: use_installation, @@ -56,82 +59,82 @@ impl Cache { ..util::base_options(lossy) }; - let config = - { - let home_env = &home_env; - let xdg_config_home_env = &xdg_config_home_env; - let git_prefix = &git_prefix; - let metas = [ - git_config::source::Kind::GitInstallation, - git_config::source::Kind::System, - git_config::source::Kind::Global, - ] - .iter() - .flat_map(|kind| kind.sources()) - .filter_map(|source| { - match source { - git_config::Source::GitInstallation if !use_installation => return None, - git_config::Source::System if !use_system => return None, - git_config::Source::Git if !use_git => return None, - git_config::Source::User if !use_user => return None, - _ => {} - } - source - .storage_location(&mut |name| { - match name { - git_ if git_.starts_with("GIT_") => Some(git_prefix), - "XDG_CONFIG_HOME" => Some(xdg_config_home_env), - "HOME" => Some(home_env), - _ => None, - } - .and_then(|perm| std::env::var_os(name).and_then(|val| perm.check_opt(val))) - }) - .map(|p| (source, p.into_owned())) - }) - .map(|(source, path)| git_config::file::Metadata { - path: Some(path), - source: *source, - level: 0, - trust: git_sec::Trust::Full, - }); - - let err_on_nonexisting_paths = false; - let mut globals = git_config::File::from_paths_metadata_buf( - metas, - &mut buf, - err_on_nonexisting_paths, - git_config::file::init::Options { - includes: git_config::file::includes::Options::no_follow(), - ..options - }, - ) - .map_err(|err| match err { - git_config::file::init::from_paths::Error::Init(err) => Error::from(err), - git_config::file::init::from_paths::Error::Io(err) => err.into(), - })? - .unwrap_or_default(); - - globals.append(git_dir_config); - globals.resolve_includes(options)?; - if use_env { - globals.append(git_config::File::from_env(options)?.unwrap_or_default()); - } - if !cli_config_overrides.is_empty() { - crate::config::overrides::append(&mut globals, cli_config_overrides, git_config::Source::Cli) - .map_err(|err| Error::ConfigOverrides { - err, - source: git_config::Source::Cli, - })?; - } - if !api_config_overrides.is_empty() { - crate::config::overrides::append(&mut globals, api_config_overrides, git_config::Source::Api) - .map_err(|err| Error::ConfigOverrides { - err, - source: git_config::Source::Api, - })?; + let config = { + let home_env = &home_env; + let xdg_config_home_env = &xdg_config_home_env; + let git_prefix = &git_prefix; + let metas = [ + git_config::source::Kind::GitInstallation, + git_config::source::Kind::System, + git_config::source::Kind::Global, + ] + .iter() + .flat_map(|kind| kind.sources()) + .filter_map(|source| { + match source { + git_config::Source::GitInstallation if !use_installation => return None, + git_config::Source::System if !use_system => return None, + git_config::Source::Git if !use_git => return None, + git_config::Source::User if !use_user => return None, + _ => {} } - globals - }; + source + .storage_location(&mut |name| { + match name { + git_ if git_.starts_with("GIT_") => Some(git_prefix), + "XDG_CONFIG_HOME" => Some(xdg_config_home_env), + "HOME" => Some(home_env), + _ => None, + } + .and_then(|perm| std::env::var_os(name).and_then(|val| perm.check_opt(val))) + }) + .map(|p| (source, p.into_owned())) + }) + .map(|(source, path)| git_config::file::Metadata { + path: Some(path), + source: *source, + level: 0, + trust: git_sec::Trust::Full, + }); + + let err_on_nonexisting_paths = false; + let mut globals = git_config::File::from_paths_metadata_buf( + metas, + &mut buf, + err_on_nonexisting_paths, + git_config::file::init::Options { + includes: git_config::file::includes::Options::no_follow(), + ..options + }, + ) + .map_err(|err| match err { + git_config::file::init::from_paths::Error::Init(err) => Error::from(err), + git_config::file::init::from_paths::Error::Io(err) => err.into(), + })? + .unwrap_or_default(); + + globals.append(git_dir_config); + globals.resolve_includes(options)?; + if use_env { + globals.append(git_config::File::from_env(options)?.unwrap_or_default()); + } + if !cli_config_overrides.is_empty() { + crate::config::overrides::append(&mut globals, cli_config_overrides, git_config::Source::Cli, |_| None) + .map_err(|err| Error::ConfigOverrides { + err, + source: git_config::Source::Cli, + })?; + } + if !api_config_overrides.is_empty() { + crate::config::overrides::append(&mut globals, api_config_overrides, git_config::Source::Api, |_| None) + .map_err(|err| Error::ConfigOverrides { + err, + source: git_config::Source::Api, + })?; + } + apply_environment_overrides(&mut globals, *git_prefix, http_transport)?; + globals + }; let hex_len = util::parse_core_abbrev(&config, object_hash).with_leniency(lenient_config)?; @@ -208,3 +211,144 @@ impl Cache { Ok(()) } } + +impl crate::Repository { + /// Causes our configuration to re-read cached values which will also be applied to the repository in-memory state if applicable. + /// + /// Similar to `reread_values_and_clear_caches_replacing_config()`, but works on the existing instance instead of a passed + /// in one that it them makes the default. + #[cfg(feature = "blocking-network-client")] + pub(crate) fn reread_values_and_clear_caches(&mut self) -> Result<(), Error> { + self.config.reread_values_and_clear_caches()?; + self.apply_changed_values(); + Ok(()) + } + + /// Replace our own configuration with `config` and re-read all cached values, and apply them to select in-memory instances. + pub(crate) fn reread_values_and_clear_caches_replacing_config( + &mut self, + config: crate::Config, + ) -> Result<(), Error> { + self.config.reread_values_and_clear_caches_replacing_config(config)?; + self.apply_changed_values(); + Ok(()) + } + + fn apply_changed_values(&mut self) { + self.refs.write_reflog = util::reflog_or_default(self.config.reflog, self.work_dir().is_some()); + } +} + +fn apply_environment_overrides( + config: &mut File<'static>, + git_prefix: Permission, + http_transport: Permission, +) -> Result<(), Error> { + fn var_as_bstring(var: &str, perm: Permission) -> Option { + perm.check_opt(var) + .and_then(std::env::var_os) + .and_then(|val| git_path::os_string_into_bstring(val).ok()) + } + + let mut env_override = git_config::File::new(git_config::file::Metadata::from(git_config::Source::EnvOverride)); + { + let mut section = env_override + .new_section("http", None) + .expect("statically known valid section name"); + for (var, key, permission) in [ + ("GIT_HTTP_LOW_SPEED_LIMIT", "lowSpeedLimit", git_prefix), + ("GIT_HTTP_LOW_SPEED_TIME", "lowSpeedTime", git_prefix), + ("GIT_HTTP_USER_AGENT", "userAgent", git_prefix), + ("GIT_HTTP_PROXY_AUTHMETHOD", "proxyAuthMethod", git_prefix), + ("all_proxy", "all-proxy-lower", http_transport), + ("ALL_PROXY", "all-proxy", http_transport), + ] { + if let Some(value) = var_as_bstring(var, permission) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("https".into()))) + .expect("statically known valid section name"); + + for (var, key) in [("HTTPS_PROXY", "proxy"), ("https_proxy", "proxy")] { + if let Some(value) = var_as_bstring(var, http_transport) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("allow".into()))) + .expect("statically known valid section name"); + + for (var, key) in [("GIT_PROTOCOL_FROM_USER", "protocolFromUser")] { + if let Some(value) = var_as_bstring(var, http_transport) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("http".into()))) + .expect("statically known valid section name"); + + for (var, key, permission) in [ + ("ALL_PROXY", "allProxy", http_transport), + ("all_proxy", "allProxy", http_transport), + ("NO_PROXY", "noProxy", http_transport), + ("no_proxy", "noProxy", http_transport), + ("http_proxy", "proxy", http_transport), + ("GIT_CURL_VERBOSE", "verbose", git_prefix), + ] { + if let Some(value) = var_as_bstring(var, permission) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + if !env_override.is_void() { + config.append(env_override); + } + Ok(()) +} diff --git a/git-repository/src/config/cache/util.rs b/git-repository/src/config/cache/util.rs index 88bcff2f276..b74938ddbc3 100644 --- a/git-repository/src/config/cache/util.rs +++ b/git-repository/src/config/cache/util.rs @@ -93,6 +93,17 @@ where } } +pub(crate) fn reflog_or_default( + config_reflog: Option, + has_worktree: bool, +) -> git_ref::store::WriteReflog { + config_reflog.unwrap_or_else(|| { + has_worktree + .then(|| git_ref::store::WriteReflog::Normal) + .unwrap_or(git_ref::store::WriteReflog::Disable) + }) +} + pub(crate) fn parse_core_abbrev( config: &git_config::File<'static>, object_hash: git_hash::Kind, diff --git a/git-repository/src/config/overrides.rs b/git-repository/src/config/overrides.rs index 57d23386a8b..79fdf798609 100644 --- a/git-repository/src/config/overrides.rs +++ b/git-repository/src/config/overrides.rs @@ -2,7 +2,7 @@ use std::convert::TryFrom; use crate::bstr::{BStr, BString, ByteSlice}; -/// The error returned by [SnapshotMut::apply_cli_overrides()][crate::config::SnapshotMut::apply_cli_overrides()]. +/// The error returned by [SnapshotMut::apply_cli_overrides()][crate::config::SnapshotMut::append_config()]. #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { @@ -21,6 +21,7 @@ pub(crate) fn append( config: &mut git_config::File<'static>, values: impl IntoIterator>, source: git_config::Source, + mut make_comment: impl FnMut(&BStr) -> Option, ) -> Result<(), Error> { let mut file = git_config::File::new(git_config::file::Metadata::from(source)); for key_value in values { @@ -31,13 +32,17 @@ pub(crate) fn append( let key = git_config::parse::key(key.to_str().map_err(|_| Error::InvalidKey { input: key.into() })?) .ok_or_else(|| Error::InvalidKey { input: key.into() })?; let mut section = file.section_mut_or_create_new(key.section_name, key.subsection_name)?; - section.push( + let key = git_config::parse::section::Key::try_from(key.value_name.to_owned()).map_err(|err| Error::SectionKey { source: err, key: key.value_name.into(), - })?, - value.map(|v| v.as_bstr()), - ); + })?; + let comment = make_comment(key_value); + let value = value.map(|v| v.as_bstr()); + match comment { + Some(comment) => section.push_with_comment(key, value, &**comment), + None => section.push(key, value), + } } config.append(file); Ok(()) diff --git a/git-repository/src/config/snapshot/_impls.rs b/git-repository/src/config/snapshot/_impls.rs index 7d7e98caed8..ffbb917ca8c 100644 --- a/git-repository/src/config/snapshot/_impls.rs +++ b/git-repository/src/config/snapshot/_impls.rs @@ -51,7 +51,7 @@ impl Deref for Snapshot<'_> { type Target = git_config::File<'static>; fn deref(&self) -> &Self::Target { - &self.plumbing() + self.plumbing() } } diff --git a/git-repository/src/config/snapshot/access.rs b/git-repository/src/config/snapshot/access.rs index 1232cba6c2f..14f0326dc43 100644 --- a/git-repository/src/config/snapshot/access.rs +++ b/git-repository/src/config/snapshot/access.rs @@ -86,7 +86,7 @@ impl<'repo> SnapshotMut<'repo> { values: impl IntoIterator>, source: git_config::Source, ) -> Result<&mut Self, crate::config::overrides::Error> { - crate::config::overrides::append(&mut self.config, values, source)?; + crate::config::overrides::append(&mut self.config, values, source, |v| Some(format!("-c {v}").into()))?; Ok(self) } /// Apply all changes made to this instance. diff --git a/git-repository/src/open.rs b/git-repository/src/open.rs deleted file mode 100644 index 050c505fdce..00000000000 --- a/git-repository/src/open.rs +++ /dev/null @@ -1,604 +0,0 @@ -use std::path::PathBuf; - -use git_features::threading::OwnShared; - -use crate::{ - bstr::BString, config, config::cache::interpolate_context, permission, Permissions, Repository, - ThreadSafeRepository, -}; - -/// A way to configure the usage of replacement objects, see `git replace`. -#[derive(Debug, Clone)] -pub enum ReplacementObjects { - /// Allow replacement objects and configure the ref prefix the standard environment variable `GIT_REPLACE_REF_BASE`, - /// or default to the standard `refs/replace/` prefix. - UseWithEnvironmentRefPrefixOrDefault { - /// If true, default true, a standard environment variable `GIT_NO_REPLACE_OBJECTS` to disable replacement objects entirely. - allow_disable_via_environment: bool, - }, - /// Use replacement objects and configure the prefix yourself. - UseWithRefPrefix { - /// The ref prefix to use, like `refs/alternative/` - note the trailing slash. - prefix: PathBuf, - /// If true, default true, a standard environment variable `GIT_NO_REPLACE_OBJECTS` - allow_disable_via_environment: bool, - }, - /// Do not use replacement objects at all. - Disable, -} - -impl Default for ReplacementObjects { - fn default() -> Self { - ReplacementObjects::UseWithEnvironmentRefPrefixOrDefault { - allow_disable_via_environment: true, - } - } -} - -impl ReplacementObjects { - fn refs_prefix(self) -> Option { - use ReplacementObjects::*; - let is_disabled = |allow_env: bool| allow_env && std::env::var_os("GIT_NO_REPLACE_OBJECTS").is_some(); - match self { - UseWithEnvironmentRefPrefixOrDefault { - allow_disable_via_environment, - } => { - if is_disabled(allow_disable_via_environment) { - return None; - }; - PathBuf::from(std::env::var("GIT_REPLACE_REF_BASE").unwrap_or_else(|_| "refs/replace/".into())).into() - } - UseWithRefPrefix { - prefix, - allow_disable_via_environment, - } => { - if is_disabled(allow_disable_via_environment) { - return None; - }; - prefix.into() - } - Disable => None, - } - } -} - -/// The options used in [`ThreadSafeRepository::open_opts`] -#[derive(Clone)] -pub struct Options { - pub(crate) object_store_slots: git_odb::store::init::Slots, - pub(crate) replacement_objects: ReplacementObjects, - /// Define what is allowed while openeing a repository. - pub permissions: Permissions, - pub(crate) git_dir_trust: Option, - /// Warning: this one is copied to to config::Cache - don't change it after repo open or keep in sync. - pub(crate) filter_config_section: Option bool>, - pub(crate) lossy_config: Option, - pub(crate) lenient_config: bool, - pub(crate) bail_if_untrusted: bool, - pub(crate) api_config_overrides: Vec, - pub(crate) cli_config_overrides: Vec, - /// Internal to pass an already obtained CWD on to where it may also be used. This avoids the CWD being queried more than once per repo. - pub(crate) current_dir: Option, -} - -impl Default for Options { - fn default() -> Self { - Options { - object_store_slots: Default::default(), - replacement_objects: Default::default(), - permissions: Default::default(), - git_dir_trust: None, - filter_config_section: None, - lossy_config: None, - lenient_config: true, - bail_if_untrusted: false, - api_config_overrides: Vec::new(), - cli_config_overrides: Vec::new(), - current_dir: None, - } - } -} - -#[derive(Default, Clone)] -pub(crate) struct EnvironmentOverrides { - /// An override of the worktree typically from the environment, and overrides even worktree dirs set as parameter. - /// - /// This emulates the way git handles this override. - worktree_dir: Option, - /// An override for the .git directory, typically from the environment. - /// - /// If set, the passed in `git_dir` parameter will be ignored in favor of this one. - git_dir: Option, -} - -impl EnvironmentOverrides { - fn from_env() -> Result { - let mut worktree_dir = None; - if let Some(path) = std::env::var_os("GIT_WORK_TREE") { - worktree_dir = PathBuf::from(path).into(); - } - let mut git_dir = None; - if let Some(path) = std::env::var_os("GIT_DIR") { - git_dir = PathBuf::from(path).into(); - } - Ok(EnvironmentOverrides { worktree_dir, git_dir }) - } -} - -/// Instantiation -impl Options { - /// Options configured to prevent accessing anything else than the repository configuration file, prohibiting - /// accessing the environment or spreading beyond the git repository location. - pub fn isolated() -> Self { - Options::default().permissions(Permissions::isolated()) - } -} - -/// Generic modification -impl Options { - /// An adapter to allow calling any builder method on this instance despite only having a mutable reference. - pub fn modify(&mut self, f: impl FnOnce(Self) -> Self) { - *self = f(std::mem::take(self)); - } -} - -/// Builder methods -impl Options { - /// Apply the given configuration `values` like `init.defaultBranch=special` or `core.bool-implicit-true` in memory to as early - /// as the configuration is initialized to allow affecting the repository instantiation phase, both on disk or when opening. - /// The configuration is marked with [source API][git_config::Source::Api]. - pub fn config_overrides(mut self, values: impl IntoIterator>) -> Self { - self.api_config_overrides = values.into_iter().map(Into::into).collect(); - self - } - - /// Set configuration values of the form `core.abbrev=5` or `remote.origin.url = foo` or `core.bool-implicit-true` for application - /// as CLI overrides to the repository configuration, marked with [source CLI][git_config::Source::Cli]. - /// These are equivalent to CLI overrides passed with `-c` in `git`, for example. - pub fn cli_overrides(mut self, values: impl IntoIterator>) -> Self { - self.cli_config_overrides = values.into_iter().map(Into::into).collect(); - self - } - - /// Set the amount of slots to use for the object database. It's a value that doesn't need changes on the client, typically, - /// but should be controlled on the server. - pub fn object_store_slots(mut self, slots: git_odb::store::init::Slots) -> Self { - self.object_store_slots = slots; - self - } - - // TODO: tests - /// Configure replacement objects, see the [`ReplacementObjects`] type for details. - pub fn replacement_objects(mut self, config: ReplacementObjects) -> Self { - self.replacement_objects = config; - self - } - - // TODO: tests - /// Set the given permissions, which are typically derived by a `Trust` level. - pub fn permissions(mut self, permissions: Permissions) -> Self { - self.permissions = permissions; - self - } - - /// Set the trust level of the `.git` directory we are about to open. - /// - /// This can be set manually to force trust even though otherwise it might - /// not be fully trusted, leading to limitations in how configuration files - /// are interpreted. - /// - /// If not called explicitly, it will be determined by looking at its - /// ownership via [`git_sec::Trust::from_path_ownership()`]. - /// - /// # Security Warning - /// - /// Use with extreme care and only if it's absolutely known that the repository - /// is always controlled by the desired user. Using this capability _only_ saves - /// a permission check and only so if the [`open()`][Self::open()] method is used, - /// as opposed to discovery. - pub fn with(mut self, trust: git_sec::Trust) -> Self { - self.git_dir_trust = trust.into(); - self - } - - /// If true, default false, and if the repository's trust level is not `Full` - /// (see [`with()`][Self::with()] for more), then the open operation will fail. - /// - /// Use this to mimic `git`s way of handling untrusted repositories. Note that `gitoxide` solves - /// this by not using configuration from untrusted sources and by generally being secured against - /// doctored input files which at worst could cause out-of-memory at the time of writing. - pub fn bail_if_untrusted(mut self, toggle: bool) -> Self { - self.bail_if_untrusted = toggle; - self - } - - /// Set the filter which determines if a configuration section can be used to read values from, - /// hence it returns true if it is eligible. - /// - /// The default filter selects sections whose trust level is [`full`][git_sec::Trust::Full] or - /// whose source is not [`repository-local`][git_config::source::Kind::Repository]. - pub fn filter_config_section(mut self, filter: fn(&git_config::file::Metadata) -> bool) -> Self { - self.filter_config_section = Some(filter); - self - } - - /// By default, in release mode configuration will be read without retaining non-essential information like - /// comments or whitespace to optimize lookup performance. - /// - /// Some application might want to toggle this to false in they want to display or edit configuration losslessly - /// with all whitespace and comments included. - pub fn lossy_config(mut self, toggle: bool) -> Self { - self.lossy_config = toggle.into(); - self - } - - /// If set, default is false, invalid configuration values will cause an error even if these can safely be defaulted. - /// - /// This is recommended for all applications that prefer correctness over usability. - /// `git` itself defaults to strict configuration mode, flagging incorrect configuration immediately. - pub fn strict_config(mut self, toggle: bool) -> Self { - self.lenient_config = !toggle; - self - } - - /// Open a repository at `path` with the options set so far. - pub fn open(self, path: impl Into) -> Result { - ThreadSafeRepository::open_opts(path, self) - } -} - -impl git_sec::trust::DefaultForLevel for Options { - fn default_for_level(level: git_sec::Trust) -> Self { - match level { - git_sec::Trust::Full => Options { - object_store_slots: Default::default(), - replacement_objects: Default::default(), - permissions: Permissions::default_for_level(level), - git_dir_trust: git_sec::Trust::Full.into(), - filter_config_section: Some(config::section::is_trusted), - lossy_config: None, - bail_if_untrusted: false, - lenient_config: true, - api_config_overrides: Vec::new(), - cli_config_overrides: Vec::new(), - current_dir: None, - }, - git_sec::Trust::Reduced => Options { - object_store_slots: git_odb::store::init::Slots::Given(32), // limit resource usage - replacement_objects: ReplacementObjects::Disable, // don't be tricked into seeing manufactured objects - permissions: Permissions::default_for_level(level), - git_dir_trust: git_sec::Trust::Reduced.into(), - filter_config_section: Some(config::section::is_trusted), - bail_if_untrusted: false, - lenient_config: true, - lossy_config: None, - api_config_overrides: Vec::new(), - cli_config_overrides: Vec::new(), - current_dir: None, - }, - } - } -} - -/// The error returned by [`crate::open()`]. -#[derive(Debug, thiserror::Error)] -#[allow(missing_docs)] -pub enum Error { - #[error("Failed to load the git configuration")] - Config(#[from] config::Error), - #[error(transparent)] - NotARepository(#[from] git_discover::is_git::Error), - #[error(transparent)] - Io(#[from] std::io::Error), - #[error("The git directory at '{}' is considered unsafe as it's not owned by the current user.", .path.display())] - UnsafeGitDir { path: PathBuf }, - #[error(transparent)] - EnvironmentAccessDenied(#[from] permission::env_var::resource::Error), -} - -impl ThreadSafeRepository { - /// Open a git repository at the given `path`, possibly expanding it to `path/.git` if `path` is a work tree dir. - pub fn open(path: impl Into) -> Result { - Self::open_opts(path, Options::default()) - } - - /// Open a git repository at the given `path`, possibly expanding it to `path/.git` if `path` is a work tree dir, and use - /// `options` for fine-grained control. - /// - /// Note that you should use [`crate::discover()`] if security should be adjusted by ownership. - pub fn open_opts(path: impl Into, mut options: Options) -> Result { - let (path, kind) = { - let path = path.into(); - match git_discover::is_git(&path) { - Ok(kind) => (path, kind), - Err(_err) => { - let git_dir = path.join(git_discover::DOT_GIT_DIR); - git_discover::is_git(&git_dir).map(|kind| (git_dir, kind))? - } - } - }; - let cwd = std::env::current_dir()?; - let (git_dir, worktree_dir) = git_discover::repository::Path::from_dot_git_dir(path, kind, &cwd) - .expect("we have sanitized path with is_git()") - .into_repository_and_work_tree_directories(); - if options.git_dir_trust.is_none() { - options.git_dir_trust = git_sec::Trust::from_path_ownership(&git_dir)?.into(); - } - options.current_dir = Some(cwd); - ThreadSafeRepository::open_from_paths(git_dir, worktree_dir, options) - } - - /// Try to open a git repository in `fallback_directory` (can be worktree or `.git` directory) only if there is no override - /// from of the `gitdir` using git environment variables. - /// - /// Use the `trust_map` to apply options depending in the trust level for `directory` or the directory it's overridden with. - /// The `.git` directory whether given or computed is used for trust checks. - /// - /// Note that this will read various `GIT_*` environment variables to check for overrides, and is probably most useful when implementing - /// custom hooks. - // TODO: tests, with hooks, GIT_QUARANTINE for ref-log and transaction control (needs git-sec support to remove write access in git-ref) - // TODO: The following vars should end up as overrides of the respective configuration values (see git-config). - // GIT_HTTP_PROXY_AUTHMETHOD, GIT_PROXY_SSL_CERT, GIT_PROXY_SSL_KEY, GIT_PROXY_SSL_CERT_PASSWORD_PROTECTED. - // GIT_PROXY_SSL_CAINFO, GIT_SSL_VERSION, GIT_SSL_CIPHER_LIST, GIT_HTTP_MAX_REQUESTS, GIT_CURL_FTP_NO_EPSV, - // GIT_HTTP_LOW_SPEED_LIMIT, GIT_HTTP_LOW_SPEED_TIME, GIT_HTTP_USER_AGENT, - // no_proxy, NO_PROXY, http_proxy, HTTPS_PROXY, https_proxy, ALL_PROXY, all_proxy - pub fn open_with_environment_overrides( - fallback_directory: impl Into, - trust_map: git_sec::trust::Mapping, - ) -> Result { - let overrides = EnvironmentOverrides::from_env()?; - let (path, path_kind): (PathBuf, _) = match overrides.git_dir { - Some(git_dir) => git_discover::is_git(&git_dir).map(|kind| (git_dir, kind))?, - None => { - let fallback_directory = fallback_directory.into(); - git_discover::is_git(&fallback_directory).map(|kind| (fallback_directory, kind))? - } - }; - - let cwd = std::env::current_dir()?; - let (git_dir, worktree_dir) = git_discover::repository::Path::from_dot_git_dir(path, path_kind, &cwd) - .expect("we have sanitized path with is_git()") - .into_repository_and_work_tree_directories(); - let worktree_dir = worktree_dir.or(overrides.worktree_dir); - - let git_dir_trust = git_sec::Trust::from_path_ownership(&git_dir)?; - let mut options = trust_map.into_value_by_level(git_dir_trust); - options.current_dir = Some(cwd); - ThreadSafeRepository::open_from_paths(git_dir, worktree_dir, options) - } - - pub(crate) fn open_from_paths( - git_dir: PathBuf, - mut worktree_dir: Option, - options: Options, - ) -> Result { - let Options { - git_dir_trust, - object_store_slots, - filter_config_section, - ref replacement_objects, - lossy_config, - lenient_config, - bail_if_untrusted, - permissions: Permissions { ref env, config }, - ref api_config_overrides, - ref cli_config_overrides, - ref current_dir, - } = options; - let current_dir = current_dir.as_deref().expect("BUG: current_dir must be set by caller"); - let git_dir_trust = git_dir_trust.expect("trust must be been determined by now"); - - // TODO: assure we handle the worktree-dir properly as we can have config per worktree with an extension. - // This would be something read in later as have to first check for extensions. Also this means - // that each worktree, even if accessible through this instance, has to come in its own Repository instance - // as it may have its own configuration. That's fine actually. - let common_dir = git_discover::path::from_plain_file(git_dir.join("commondir")) - .transpose()? - .map(|cd| git_dir.join(cd)); - let common_dir_ref = common_dir.as_deref().unwrap_or(&git_dir); - - let repo_config = config::cache::StageOne::new(common_dir_ref, git_dir_trust, lossy_config, lenient_config)?; - let mut refs = { - let reflog = repo_config.reflog.unwrap_or(git_ref::store::WriteReflog::Disable); - let object_hash = repo_config.object_hash; - match &common_dir { - Some(common_dir) => crate::RefStore::for_linked_worktree(&git_dir, common_dir, reflog, object_hash), - None => crate::RefStore::at(&git_dir, reflog, object_hash), - } - }; - let head = refs.find("HEAD").ok(); - let git_install_dir = crate::path::install_dir().ok(); - let home = std::env::var_os("HOME") - .map(PathBuf::from) - .and_then(|home| env.home.check_opt(home)); - - let mut filter_config_section = filter_config_section.unwrap_or(config::section::is_trusted); - let config = config::Cache::from_stage_one( - repo_config, - common_dir_ref, - head.as_ref().and_then(|head| head.target.try_name()), - filter_config_section, - git_install_dir.as_deref(), - home.as_deref(), - env.clone(), - config, - lenient_config, - api_config_overrides, - cli_config_overrides, - )?; - - if bail_if_untrusted && git_dir_trust != git_sec::Trust::Full { - check_safe_directories(&git_dir, git_install_dir.as_deref(), home.as_deref(), &config)?; - } - - // core.worktree might be used to overwrite the worktree directory - if !config.is_bare { - if let Some(wt) = config - .resolved - .path_filter("core", None, "worktree", &mut filter_config_section) - { - let wt_path = wt - .interpolate(interpolate_context(git_install_dir.as_deref(), home.as_deref())) - .map_err(config::Error::PathInterpolation)?; - worktree_dir = { - git_path::absolutize(git_dir.join(wt_path), current_dir) - .and_then(|wt| wt.as_ref().is_dir().then(|| wt.into_owned())) - } - } - } - - match worktree_dir { - None if !config.is_bare => { - worktree_dir = Some(git_dir.parent().expect("parent is always available").to_owned()); - } - Some(_) => { - // note that we might be bare even with a worktree directory - work trees don't have to be - // the parent of a non-bare repository. - } - None => {} - } - - refs.write_reflog = reflog_or_default(config.reflog, worktree_dir.is_some()); - let replacements = replacement_objects - .clone() - .refs_prefix() - .and_then(|prefix| { - let platform = refs.iter().ok()?; - let iter = platform.prefixed(&prefix).ok()?; - let prefix = prefix.to_str()?; - let replacements = iter - .filter_map(Result::ok) - .filter_map(|r: git_ref::Reference| { - let target = r.target.try_id()?.to_owned(); - let source = - git_hash::ObjectId::from_hex(r.name.as_bstr().strip_prefix(prefix.as_bytes())?).ok()?; - Some((source, target)) - }) - .collect::>(); - Some(replacements) - }) - .unwrap_or_default(); - - Ok(ThreadSafeRepository { - objects: OwnShared::new(git_odb::Store::at_opts( - common_dir_ref.join("objects"), - replacements, - git_odb::store::init::Options { - slots: object_store_slots, - object_hash: config.object_hash, - use_multi_pack_index: config.use_multi_pack_index, - current_dir: current_dir.to_owned().into(), - }, - )?), - common_dir, - refs, - work_tree: worktree_dir, - config, - // used when spawning new repositories off this one when following worktrees - linked_worktree_options: options, - index: git_features::fs::MutableSnapshot::new().into(), - }) - } -} - -impl Repository { - /// Causes our configuration to re-read cached values which will also be applied to the repository in-memory state if applicable. - /// - /// Similar to `reread_values_and_clear_caches_replacing_config()`, but works on the existing instance instead of a passed - /// in one that it them makes the default. - #[cfg(feature = "blocking-network-client")] - pub(crate) fn reread_values_and_clear_caches(&mut self) -> Result<(), config::Error> { - self.config.reread_values_and_clear_caches()?; - self.apply_changed_values(); - Ok(()) - } - - /// Replace our own configuration with `config` and re-read all cached values, and apply them to select in-memory instances. - pub(crate) fn reread_values_and_clear_caches_replacing_config( - &mut self, - config: crate::Config, - ) -> Result<(), config::Error> { - self.config.reread_values_and_clear_caches_replacing_config(config)?; - self.apply_changed_values(); - Ok(()) - } - - fn apply_changed_values(&mut self) { - self.refs.write_reflog = reflog_or_default(self.config.reflog, self.work_dir().is_some()); - } -} - -fn reflog_or_default( - config_reflog: Option, - has_worktree: bool, -) -> git_ref::store::WriteReflog { - config_reflog.unwrap_or_else(|| { - has_worktree - .then(|| git_ref::store::WriteReflog::Normal) - .unwrap_or(git_ref::store::WriteReflog::Disable) - }) -} - -fn check_safe_directories( - git_dir: &std::path::Path, - git_install_dir: Option<&std::path::Path>, - home: Option<&std::path::Path>, - config: &config::Cache, -) -> Result<(), Error> { - let mut is_safe = false; - let git_dir = match git_path::realpath(git_dir) { - Ok(p) => p, - Err(_) => git_dir.to_owned(), - }; - for safe_dir in config - .resolved - .strings_filter("safe", None, "directory", &mut |meta| { - let kind = meta.source.kind(); - kind == git_config::source::Kind::System || kind == git_config::source::Kind::Global - }) - .unwrap_or_default() - { - if safe_dir.as_ref() == "*" { - is_safe = true; - continue; - } - if safe_dir.is_empty() { - is_safe = false; - continue; - } - if !is_safe { - let safe_dir = match git_config::Path::from(std::borrow::Cow::Borrowed(safe_dir.as_ref())) - .interpolate(interpolate_context(git_install_dir, home)) - { - Ok(path) => path, - Err(_) => git_path::from_bstr(safe_dir), - }; - if safe_dir == git_dir { - is_safe = true; - continue; - } - } - } - if is_safe { - Ok(()) - } else { - Err(Error::UnsafeGitDir { path: git_dir }) - } -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn size_of_options() { - let actual = std::mem::size_of::(); - let limit = 160; - assert!( - actual <= limit, - "{} <= {}: size shouldn't change without us knowing (on windows, it's bigger)", - actual, - limit - ); - } -} diff --git a/git-repository/src/open/mod.rs b/git-repository/src/open/mod.rs new file mode 100644 index 00000000000..2150bda21ef --- /dev/null +++ b/git-repository/src/open/mod.rs @@ -0,0 +1,114 @@ +use std::path::PathBuf; + +use crate::{bstr::BString, config, permission, Permissions}; + +/// A way to configure the usage of replacement objects, see `git replace`. +#[derive(Debug, Clone)] +pub enum ReplacementObjects { + /// Allow replacement objects and configure the ref prefix the standard environment variable `GIT_REPLACE_REF_BASE`, + /// or default to the standard `refs/replace/` prefix. + UseWithEnvironmentRefPrefixOrDefault { + /// If true, default true, a standard environment variable `GIT_NO_REPLACE_OBJECTS` to disable replacement objects entirely. + allow_disable_via_environment: bool, + }, + /// Use replacement objects and configure the prefix yourself. + UseWithRefPrefix { + /// The ref prefix to use, like `refs/alternative/` - note the trailing slash. + prefix: PathBuf, + /// If true, default true, a standard environment variable `GIT_NO_REPLACE_OBJECTS` + allow_disable_via_environment: bool, + }, + /// Do not use replacement objects at all. + Disable, +} + +impl Default for ReplacementObjects { + fn default() -> Self { + ReplacementObjects::UseWithEnvironmentRefPrefixOrDefault { + allow_disable_via_environment: true, + } + } +} + +impl ReplacementObjects { + fn refs_prefix(self) -> Option { + use ReplacementObjects::*; + let is_disabled = |allow_env: bool| allow_env && std::env::var_os("GIT_NO_REPLACE_OBJECTS").is_some(); + match self { + UseWithEnvironmentRefPrefixOrDefault { + allow_disable_via_environment, + } => { + if is_disabled(allow_disable_via_environment) { + return None; + }; + PathBuf::from(std::env::var("GIT_REPLACE_REF_BASE").unwrap_or_else(|_| "refs/replace/".into())).into() + } + UseWithRefPrefix { + prefix, + allow_disable_via_environment, + } => { + if is_disabled(allow_disable_via_environment) { + return None; + }; + prefix.into() + } + Disable => None, + } + } +} + +/// The options used in [`ThreadSafeRepository::open_opts`] +#[derive(Clone)] +pub struct Options { + pub(crate) object_store_slots: git_odb::store::init::Slots, + pub(crate) replacement_objects: ReplacementObjects, + /// Define what is allowed while opening a repository. + pub permissions: Permissions, + pub(crate) git_dir_trust: Option, + /// Warning: this one is copied to to config::Cache - don't change it after repo open or keep in sync. + pub(crate) filter_config_section: Option bool>, + pub(crate) lossy_config: Option, + pub(crate) lenient_config: bool, + pub(crate) bail_if_untrusted: bool, + pub(crate) api_config_overrides: Vec, + pub(crate) cli_config_overrides: Vec, + /// Internal to pass an already obtained CWD on to where it may also be used. This avoids the CWD being queried more than once per repo. + pub(crate) current_dir: Option, +} + +/// The error returned by [`crate::open()`]. +#[derive(Debug, thiserror::Error)] +#[allow(missing_docs)] +pub enum Error { + #[error("Failed to load the git configuration")] + Config(#[from] config::Error), + #[error(transparent)] + NotARepository(#[from] git_discover::is_git::Error), + #[error(transparent)] + Io(#[from] std::io::Error), + #[error("The git directory at '{}' is considered unsafe as it's not owned by the current user.", .path.display())] + UnsafeGitDir { path: PathBuf }, + #[error(transparent)] + EnvironmentAccessDenied(#[from] permission::env_var::resource::Error), +} + +mod options; + +mod repository; + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn size_of_options() { + let actual = std::mem::size_of::(); + let limit = 160; + assert!( + actual <= limit, + "{} <= {}: size shouldn't change without us knowing (on windows, it's bigger)", + actual, + limit + ); + } +} diff --git a/git-repository/src/open/options.rs b/git-repository/src/open/options.rs new file mode 100644 index 00000000000..3ecee14227b --- /dev/null +++ b/git-repository/src/open/options.rs @@ -0,0 +1,177 @@ +use super::{Error, Options, ReplacementObjects}; +use crate::bstr::BString; +use crate::{config, Permissions, ThreadSafeRepository}; +use std::path::PathBuf; + +impl Default for Options { + fn default() -> Self { + Options { + object_store_slots: Default::default(), + replacement_objects: Default::default(), + permissions: Default::default(), + git_dir_trust: None, + filter_config_section: None, + lossy_config: None, + lenient_config: true, + bail_if_untrusted: false, + api_config_overrides: Vec::new(), + cli_config_overrides: Vec::new(), + current_dir: None, + } + } +} + +/// Instantiation +impl Options { + /// Options configured to prevent accessing anything else than the repository configuration file, prohibiting + /// accessing the environment or spreading beyond the git repository location. + pub fn isolated() -> Self { + Options::default().permissions(Permissions::isolated()) + } +} + +/// Generic modification +impl Options { + /// An adapter to allow calling any builder method on this instance despite only having a mutable reference. + pub fn modify(&mut self, f: impl FnOnce(Self) -> Self) { + *self = f(std::mem::take(self)); + } +} + +/// Builder methods +impl Options { + /// Apply the given configuration `values` like `init.defaultBranch=special` or `core.bool-implicit-true` in memory to as early + /// as the configuration is initialized to allow affecting the repository instantiation phase, both on disk or when opening. + /// The configuration is marked with [source API][git_config::Source::Api]. + pub fn config_overrides(mut self, values: impl IntoIterator>) -> Self { + self.api_config_overrides = values.into_iter().map(Into::into).collect(); + self + } + + /// Set configuration values of the form `core.abbrev=5` or `remote.origin.url = foo` or `core.bool-implicit-true` for application + /// as CLI overrides to the repository configuration, marked with [source CLI][git_config::Source::Cli]. + /// These are equivalent to CLI overrides passed with `-c` in `git`, for example. + pub fn cli_overrides(mut self, values: impl IntoIterator>) -> Self { + self.cli_config_overrides = values.into_iter().map(Into::into).collect(); + self + } + + /// Set the amount of slots to use for the object database. It's a value that doesn't need changes on the client, typically, + /// but should be controlled on the server. + pub fn object_store_slots(mut self, slots: git_odb::store::init::Slots) -> Self { + self.object_store_slots = slots; + self + } + + // TODO: tests + /// Configure replacement objects, see the [`ReplacementObjects`] type for details. + pub fn replacement_objects(mut self, config: ReplacementObjects) -> Self { + self.replacement_objects = config; + self + } + + // TODO: tests + /// Set the given permissions, which are typically derived by a `Trust` level. + pub fn permissions(mut self, permissions: Permissions) -> Self { + self.permissions = permissions; + self + } + + /// Set the trust level of the `.git` directory we are about to open. + /// + /// This can be set manually to force trust even though otherwise it might + /// not be fully trusted, leading to limitations in how configuration files + /// are interpreted. + /// + /// If not called explicitly, it will be determined by looking at its + /// ownership via [`git_sec::Trust::from_path_ownership()`]. + /// + /// # Security Warning + /// + /// Use with extreme care and only if it's absolutely known that the repository + /// is always controlled by the desired user. Using this capability _only_ saves + /// a permission check and only so if the [`open()`][Self::open()] method is used, + /// as opposed to discovery. + pub fn with(mut self, trust: git_sec::Trust) -> Self { + self.git_dir_trust = trust.into(); + self + } + + /// If true, default false, and if the repository's trust level is not `Full` + /// (see [`with()`][Self::with()] for more), then the open operation will fail. + /// + /// Use this to mimic `git`s way of handling untrusted repositories. Note that `gitoxide` solves + /// this by not using configuration from untrusted sources and by generally being secured against + /// doctored input files which at worst could cause out-of-memory at the time of writing. + pub fn bail_if_untrusted(mut self, toggle: bool) -> Self { + self.bail_if_untrusted = toggle; + self + } + + /// Set the filter which determines if a configuration section can be used to read values from, + /// hence it returns true if it is eligible. + /// + /// The default filter selects sections whose trust level is [`full`][git_sec::Trust::Full] or + /// whose source is not [`repository-local`][git_config::source::Kind::Repository]. + pub fn filter_config_section(mut self, filter: fn(&git_config::file::Metadata) -> bool) -> Self { + self.filter_config_section = Some(filter); + self + } + + /// By default, in release mode configuration will be read without retaining non-essential information like + /// comments or whitespace to optimize lookup performance. + /// + /// Some application might want to toggle this to false in they want to display or edit configuration losslessly + /// with all whitespace and comments included. + pub fn lossy_config(mut self, toggle: bool) -> Self { + self.lossy_config = toggle.into(); + self + } + + /// If set, default is false, invalid configuration values will cause an error even if these can safely be defaulted. + /// + /// This is recommended for all applications that prefer correctness over usability. + /// `git` itself defaults to strict configuration mode, flagging incorrect configuration immediately. + pub fn strict_config(mut self, toggle: bool) -> Self { + self.lenient_config = !toggle; + self + } + + /// Open a repository at `path` with the options set so far. + pub fn open(self, path: impl Into) -> Result { + ThreadSafeRepository::open_opts(path, self) + } +} + +impl git_sec::trust::DefaultForLevel for Options { + fn default_for_level(level: git_sec::Trust) -> Self { + match level { + git_sec::Trust::Full => Options { + object_store_slots: Default::default(), + replacement_objects: Default::default(), + permissions: Permissions::default_for_level(level), + git_dir_trust: git_sec::Trust::Full.into(), + filter_config_section: Some(config::section::is_trusted), + lossy_config: None, + bail_if_untrusted: false, + lenient_config: true, + api_config_overrides: Vec::new(), + cli_config_overrides: Vec::new(), + current_dir: None, + }, + git_sec::Trust::Reduced => Options { + object_store_slots: git_odb::store::init::Slots::Given(32), // limit resource usage + replacement_objects: ReplacementObjects::Disable, // don't be tricked into seeing manufactured objects + permissions: Permissions::default_for_level(level), + git_dir_trust: git_sec::Trust::Reduced.into(), + filter_config_section: Some(config::section::is_trusted), + bail_if_untrusted: false, + lenient_config: true, + lossy_config: None, + api_config_overrides: Vec::new(), + cli_config_overrides: Vec::new(), + current_dir: None, + }, + } + } +} diff --git a/git-repository/src/open/repository.rs b/git-repository/src/open/repository.rs new file mode 100644 index 00000000000..7ea5c885cf0 --- /dev/null +++ b/git-repository/src/open/repository.rs @@ -0,0 +1,281 @@ +use super::{Error, Options}; +use crate::config::cache::interpolate_context; +use crate::{config, permission, Permissions, ThreadSafeRepository}; +use git_features::threading::OwnShared; +use std::path::PathBuf; + +#[derive(Default, Clone)] +pub(crate) struct EnvironmentOverrides { + /// An override of the worktree typically from the environment, and overrides even worktree dirs set as parameter. + /// + /// This emulates the way git handles this override. + worktree_dir: Option, + /// An override for the .git directory, typically from the environment. + /// + /// If set, the passed in `git_dir` parameter will be ignored in favor of this one. + git_dir: Option, +} + +impl EnvironmentOverrides { + fn from_env() -> Result { + let mut worktree_dir = None; + if let Some(path) = std::env::var_os("GIT_WORK_TREE") { + worktree_dir = PathBuf::from(path).into(); + } + let mut git_dir = None; + if let Some(path) = std::env::var_os("GIT_DIR") { + git_dir = PathBuf::from(path).into(); + } + Ok(EnvironmentOverrides { worktree_dir, git_dir }) + } +} + +impl ThreadSafeRepository { + /// Open a git repository at the given `path`, possibly expanding it to `path/.git` if `path` is a work tree dir. + pub fn open(path: impl Into) -> Result { + Self::open_opts(path, Options::default()) + } + + /// Open a git repository at the given `path`, possibly expanding it to `path/.git` if `path` is a work tree dir, and use + /// `options` for fine-grained control. + /// + /// Note that you should use [`crate::discover()`] if security should be adjusted by ownership. + pub fn open_opts(path: impl Into, mut options: Options) -> Result { + let (path, kind) = { + let path = path.into(); + match git_discover::is_git(&path) { + Ok(kind) => (path, kind), + Err(_err) => { + let git_dir = path.join(git_discover::DOT_GIT_DIR); + git_discover::is_git(&git_dir).map(|kind| (git_dir, kind))? + } + } + }; + let cwd = std::env::current_dir()?; + let (git_dir, worktree_dir) = git_discover::repository::Path::from_dot_git_dir(path, kind, &cwd) + .expect("we have sanitized path with is_git()") + .into_repository_and_work_tree_directories(); + if options.git_dir_trust.is_none() { + options.git_dir_trust = git_sec::Trust::from_path_ownership(&git_dir)?.into(); + } + options.current_dir = Some(cwd); + ThreadSafeRepository::open_from_paths(git_dir, worktree_dir, options) + } + + /// Try to open a git repository in `fallback_directory` (can be worktree or `.git` directory) only if there is no override + /// from of the `gitdir` using git environment variables. + /// + /// Use the `trust_map` to apply options depending in the trust level for `directory` or the directory it's overridden with. + /// The `.git` directory whether given or computed is used for trust checks. + /// + /// Note that this will read various `GIT_*` environment variables to check for overrides, and is probably most useful when implementing + /// custom hooks. + // TODO: tests, with hooks, GIT_QUARANTINE for ref-log and transaction control (needs git-sec support to remove write access in git-ref) + // TODO: The following vars should end up as overrides of the respective configuration values (see git-config). + // GIT_PROXY_SSL_CERT, GIT_PROXY_SSL_KEY, GIT_PROXY_SSL_CERT_PASSWORD_PROTECTED. + // GIT_PROXY_SSL_CAINFO, GIT_SSL_VERSION, GIT_SSL_CIPHER_LIST, GIT_HTTP_MAX_REQUESTS, GIT_CURL_FTP_NO_EPSV, + pub fn open_with_environment_overrides( + fallback_directory: impl Into, + trust_map: git_sec::trust::Mapping, + ) -> Result { + let overrides = EnvironmentOverrides::from_env()?; + let (path, path_kind): (PathBuf, _) = match overrides.git_dir { + Some(git_dir) => git_discover::is_git(&git_dir).map(|kind| (git_dir, kind))?, + None => { + let fallback_directory = fallback_directory.into(); + git_discover::is_git(&fallback_directory).map(|kind| (fallback_directory, kind))? + } + }; + + let cwd = std::env::current_dir()?; + let (git_dir, worktree_dir) = git_discover::repository::Path::from_dot_git_dir(path, path_kind, &cwd) + .expect("we have sanitized path with is_git()") + .into_repository_and_work_tree_directories(); + let worktree_dir = worktree_dir.or(overrides.worktree_dir); + + let git_dir_trust = git_sec::Trust::from_path_ownership(&git_dir)?; + let mut options = trust_map.into_value_by_level(git_dir_trust); + options.current_dir = Some(cwd); + ThreadSafeRepository::open_from_paths(git_dir, worktree_dir, options) + } + + pub(crate) fn open_from_paths( + git_dir: PathBuf, + mut worktree_dir: Option, + options: Options, + ) -> Result { + let Options { + git_dir_trust, + object_store_slots, + filter_config_section, + ref replacement_objects, + lossy_config, + lenient_config, + bail_if_untrusted, + permissions: Permissions { ref env, config }, + ref api_config_overrides, + ref cli_config_overrides, + ref current_dir, + } = options; + let current_dir = current_dir.as_deref().expect("BUG: current_dir must be set by caller"); + let git_dir_trust = git_dir_trust.expect("trust must be been determined by now"); + + // TODO: assure we handle the worktree-dir properly as we can have config per worktree with an extension. + // This would be something read in later as have to first check for extensions. Also this means + // that each worktree, even if accessible through this instance, has to come in its own Repository instance + // as it may have its own configuration. That's fine actually. + let common_dir = git_discover::path::from_plain_file(git_dir.join("commondir")) + .transpose()? + .map(|cd| git_dir.join(cd)); + let common_dir_ref = common_dir.as_deref().unwrap_or(&git_dir); + + let repo_config = config::cache::StageOne::new(common_dir_ref, git_dir_trust, lossy_config, lenient_config)?; + let mut refs = { + let reflog = repo_config.reflog.unwrap_or(git_ref::store::WriteReflog::Disable); + let object_hash = repo_config.object_hash; + match &common_dir { + Some(common_dir) => crate::RefStore::for_linked_worktree(&git_dir, common_dir, reflog, object_hash), + None => crate::RefStore::at(&git_dir, reflog, object_hash), + } + }; + let head = refs.find("HEAD").ok(); + let git_install_dir = crate::path::install_dir().ok(); + let home = std::env::var_os("HOME") + .map(PathBuf::from) + .and_then(|home| env.home.check_opt(home)); + + let mut filter_config_section = filter_config_section.unwrap_or(config::section::is_trusted); + let config = config::Cache::from_stage_one( + repo_config, + common_dir_ref, + head.as_ref().and_then(|head| head.target.try_name()), + filter_config_section, + git_install_dir.as_deref(), + home.as_deref(), + env.clone(), + config, + lenient_config, + api_config_overrides, + cli_config_overrides, + )?; + + if bail_if_untrusted && git_dir_trust != git_sec::Trust::Full { + check_safe_directories(&git_dir, git_install_dir.as_deref(), home.as_deref(), &config)?; + } + + // core.worktree might be used to overwrite the worktree directory + if !config.is_bare { + if let Some(wt) = config + .resolved + .path_filter("core", None, "worktree", &mut filter_config_section) + { + let wt_path = wt + .interpolate(interpolate_context(git_install_dir.as_deref(), home.as_deref())) + .map_err(config::Error::PathInterpolation)?; + worktree_dir = { + git_path::absolutize(git_dir.join(wt_path), current_dir) + .and_then(|wt| wt.as_ref().is_dir().then(|| wt.into_owned())) + } + } + } + + match worktree_dir { + None if !config.is_bare => { + worktree_dir = Some(git_dir.parent().expect("parent is always available").to_owned()); + } + Some(_) => { + // note that we might be bare even with a worktree directory - work trees don't have to be + // the parent of a non-bare repository. + } + None => {} + } + + refs.write_reflog = config::cache::util::reflog_or_default(config.reflog, worktree_dir.is_some()); + let replacements = replacement_objects + .clone() + .refs_prefix() + .and_then(|prefix| { + let platform = refs.iter().ok()?; + let iter = platform.prefixed(&prefix).ok()?; + let prefix = prefix.to_str()?; + let replacements = iter + .filter_map(Result::ok) + .filter_map(|r: git_ref::Reference| { + let target = r.target.try_id()?.to_owned(); + let source = + git_hash::ObjectId::from_hex(r.name.as_bstr().strip_prefix(prefix.as_bytes())?).ok()?; + Some((source, target)) + }) + .collect::>(); + Some(replacements) + }) + .unwrap_or_default(); + + Ok(ThreadSafeRepository { + objects: OwnShared::new(git_odb::Store::at_opts( + common_dir_ref.join("objects"), + replacements, + git_odb::store::init::Options { + slots: object_store_slots, + object_hash: config.object_hash, + use_multi_pack_index: config.use_multi_pack_index, + current_dir: current_dir.to_owned().into(), + }, + )?), + common_dir, + refs, + work_tree: worktree_dir, + config, + // used when spawning new repositories off this one when following worktrees + linked_worktree_options: options, + index: git_features::fs::MutableSnapshot::new().into(), + }) + } +} + +fn check_safe_directories( + git_dir: &std::path::Path, + git_install_dir: Option<&std::path::Path>, + home: Option<&std::path::Path>, + config: &config::Cache, +) -> Result<(), Error> { + let mut is_safe = false; + let git_dir = match git_path::realpath(git_dir) { + Ok(p) => p, + Err(_) => git_dir.to_owned(), + }; + for safe_dir in config + .resolved + .strings_filter("safe", None, "directory", &mut |meta| { + let kind = meta.source.kind(); + kind == git_config::source::Kind::System || kind == git_config::source::Kind::Global + }) + .unwrap_or_default() + { + if safe_dir.as_ref() == "*" { + is_safe = true; + continue; + } + if safe_dir.is_empty() { + is_safe = false; + continue; + } + if !is_safe { + let safe_dir = match git_config::Path::from(std::borrow::Cow::Borrowed(safe_dir.as_ref())) + .interpolate(interpolate_context(git_install_dir, home)) + { + Ok(path) => path, + Err(_) => git_path::from_bstr(safe_dir), + }; + if safe_dir == git_dir { + is_safe = true; + continue; + } + } + } + if is_safe { + Ok(()) + } else { + Err(Error::UnsafeGitDir { path: git_dir }) + } +} diff --git a/git-repository/src/remote/url/scheme_permission.rs b/git-repository/src/remote/url/scheme_permission.rs index db8d7aeefba..d73b46c1810 100644 --- a/git-repository/src/remote/url/scheme_permission.rs +++ b/git-repository/src/remote/url/scheme_permission.rs @@ -55,13 +55,13 @@ pub(crate) struct SchemePermission { /// Init impl SchemePermission { + /// NOTE: _intentionally without leniency_ pub fn from_config( config: &git_config::File<'static>, - git_prefix: git_sec::Permission, mut filter: fn(&git_config::file::Metadata) -> bool, ) -> Result { let allow: Option = config - .string_filter("protocol", None, "allow", &mut filter) + .string_filter_by_key("protocol.allow", &mut filter) .map(Allow::try_from) .transpose() .map_err(|invalid| init::Error::InvalidConfiguration { @@ -100,9 +100,9 @@ impl SchemePermission { }; let user_allowed = saw_user.then(|| { - std::env::var_os("GIT_PROTOCOL_FROM_USER") - .and_then(|val| git_prefix.check_opt(val)) - .map_or(true, |val| val == "1") + config + .string_filter_by_key("gitoxide.allow.protocolFromUser", &mut filter) + .map_or(true, |val| val.as_ref() == "1") }); Ok(SchemePermission { allow, diff --git a/git-repository/src/repository/config/transport.rs b/git-repository/src/repository/config/transport.rs index 7b07ed83d45..00fea081d8e 100644 --- a/git-repository/src/repository/config/transport.rs +++ b/git-repository/src/repository/config/transport.rs @@ -64,6 +64,10 @@ impl crate::Repository { .with_leniency(lenient) } + fn cow_bstr(v: &str) -> Cow<'_, BStr> { + Cow::Borrowed(v.into()) + } + fn integer( config: &git_config::File<'static>, lenient: bool, @@ -88,13 +92,8 @@ impl crate::Repository { where T: TryFrom, { - let git_config::parse::Key { - section_name, - subsection_name, - value_name, - } = git_config::parse::key(key).expect("valid key statically known"); config - .integer_filter(section_name, subsection_name, value_name, &mut filter) + .integer_filter_by_key(key, &mut filter) .transpose() .map_err(|err| crate::config::transport::Error::ConfigValue { source: err, key }) .with_leniency(lenient)? @@ -166,10 +165,10 @@ impl crate::Repository { opts.extra_headers = { let mut headers = Vec::new(); for header in config - .strings_filter("http", None, "extraHeader", &mut trusted_only) + .strings_filter_by_key("http.extraHeader", &mut trusted_only) .unwrap_or_default() .into_iter() - .map(|v| try_cow_to_string(v, lenient, Cow::Borrowed("http.extraHeader".into()))) + .map(|v| try_cow_to_string(v, lenient, cow_bstr("http.extraHeader"))) { let header = header?; if let Some(header) = header { @@ -217,12 +216,41 @@ impl crate::Repository { .map(|v| (v, Cow::Owned(format!("remote.{name}.proxy").into()))) }) .or_else(|| { + let key = "http.proxy"; + let http_proxy = config + .string_filter_by_key(key, &mut trusted_only) + .map(|v| (v, cow_bstr(key))) + .or_else(|| { + let key = "gitoxide.http.proxy"; + config + .string_filter_by_key(key, &mut trusted_only) + .map(|v| (v, cow_bstr(key))) + }); + if url.scheme == Https { + http_proxy.or_else(|| { + let key = "gitoxide.https.proxy"; + config + .string_filter_by_key(key, &mut trusted_only) + .map(|v| (v, cow_bstr(key))) + }) + } else { + http_proxy + } + }) + .or_else(|| { + let key = "gitoxide.http.allProxy"; config - .string_filter("http", None, "proxy", &mut trusted_only) - .map(|v| (v, Cow::Borrowed("http.proxy".into()))) + .string_filter_by_key(key, &mut trusted_only) + .map(|v| (v, cow_bstr(key))) }), lenient, )?; + opts.no_proxy = config + .string_filter_by_key("gitoxide.http.noProxy", &mut trusted_only) + .and_then(|v| { + try_cow_to_string(v, lenient, Cow::Borrowed("gitoxide.http.noProxy".into())).transpose() + }) + .transpose()?; opts.proxy_auth_method = proxy_auth_method( remote_name .and_then(|name| { @@ -232,7 +260,7 @@ impl crate::Repository { }) .or_else(|| { config - .string_filter("http", None, "proxyAuthMethod", &mut trusted_only) + .string_filter_by_key("http.proxyAuthMethod", &mut trusted_only) .map(|v| (v, Cow::Borrowed("http.proxyAuthMethod".into()))) }), lenient, @@ -257,10 +285,17 @@ impl crate::Repository { integer_opt(config, lenient, "gitoxide.http.connectTimeout", "u64", trusted_only)? .map(std::time::Duration::from_millis); opts.user_agent = config - .string_filter("http", None, "userAgent", &mut trusted_only) + .string_filter_by_key("http.userAgent", &mut trusted_only) .and_then(|v| try_cow_to_string(v, lenient, Cow::Borrowed("http.userAgent".into())).transpose()) .transpose()? .or_else(|| Some(crate::env::agent().into())); + let key = "gitoxide.http.verbose"; + opts.verbose = config + .boolean_filter_by_key(key, &mut trusted_only) + .transpose() + .with_leniency(lenient) + .map_err(|err| crate::config::transport::Error::ConfigValue { source: err, key })? + .unwrap_or_default(); Ok(Some(Box::new(opts))) } diff --git a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz index 118402096ae..fde74bd6e42 100644 --- a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz +++ b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:3687c843a39942197ab8a31896b3a6c453295de7bb8d031642eb34b292a272da -size 10816 +oid sha256:607f9cd2af225da2cff5640855d557b363949bd667454e772ae49a63deb21831 +size 13284 diff --git a/git-repository/tests/fixtures/make_config_repos.sh b/git-repository/tests/fixtures/make_config_repos.sh index caca1acbd04..f4659a5d793 100644 --- a/git-repository/tests/fixtures/make_config_repos.sh +++ b/git-repository/tests/fixtures/make_config_repos.sh @@ -28,6 +28,12 @@ git clone --shared http-config http-remote-override git config remote.origin.proxyAuthMethod negotiate ) +git init http-no-proxy +(cd http-no-proxy + git config gitoxide.http.noProxy "no validation done here" +) + + git init http-proxy-empty (cd http-proxy-empty git config http.followRedirects false @@ -36,11 +42,55 @@ git init http-proxy-empty git config --add http.proxy "" # a value override disabling it later ) +git init https-proxy-only +(cd https-proxy-only + git config gitoxide.https.proxy https +) + +git init gitoxide-http-proxy-only +(cd gitoxide-http-proxy-only + git config gitoxide.http.proxy http-fallback +) + +git init gitoxide-all-proxy-only +(cd gitoxide-all-proxy-only + git config gitoxide.http.allProxy all-proxy-fallback +) + +git init gitoxide-all-proxy +(cd gitoxide-all-proxy + git config http.proxy http + git config gitoxide.http.allProxy all-proxy-fallback +) + +git init gitoxide-http-proxy +(cd gitoxide-http-proxy + git config gitoxide.http.proxy http-fallback + git config http.proxy http +) + +git init https-proxy +(cd https-proxy + git config gitoxide.https.proxy https + git config --add http.proxy "http" # only for HTTP urls +) + +git init https-proxy-empty +(cd https-proxy-empty + git config gitoxide.https.proxy https + git config --add gitoxide.https.proxy "" # empty strings disable it +) + git init http-proxy-auto-prefix (cd http-proxy-auto-prefix git config http.proxy localhost:9090 # http:// is prefixed automatically ) +git init http-verbose +(cd http-verbose + git config gitoxide.http.verbose true +) + git init http-proxy-authenticated (cd http-proxy-authenticated git config http.proxy user@localhost:9090 diff --git a/git-repository/tests/repository/config/transport_options.rs b/git-repository/tests/repository/config/transport_options.rs index 0bfd5075e21..d3786f96321 100644 --- a/git-repository/tests/repository/config/transport_options.rs +++ b/git-repository/tests/repository/config/transport_options.rs @@ -11,9 +11,13 @@ mod http { git::open_opts(dir.join(name), git::open::Options::isolated()).unwrap() } - fn http_options(repo: &git::Repository, remote_name: Option<&str>) -> git_transport::client::http::Options { + fn http_options( + repo: &git::Repository, + remote_name: Option<&str>, + url: &str, + ) -> git_transport::client::http::Options { let opts = repo - .transport_options("https://example.com/does/not/matter", remote_name.map(Into::into)) + .transport_options(url, remote_name.map(Into::into)) .expect("valid configuration") .expect("configuration available for http"); opts.downcast_ref::() @@ -29,7 +33,7 @@ mod http { proxy_auth_method, follow_redirects, .. - } = http_options(&repo, Some("origin")); + } = http_options(&repo, Some("origin"), "https://example.com/does/not/matter"); assert_eq!(proxy_auth_method, ProxyAuthMethod::Negotiate); assert_eq!(proxy.as_deref(), Some("http://overridden")); @@ -45,14 +49,14 @@ mod http { low_speed_limit_bytes_per_second, low_speed_time_seconds, proxy, - no_proxy: _, + no_proxy, proxy_auth_method, proxy_authenticate, user_agent, connect_timeout, - verbose: _, + verbose, backend, - } = http_options(&repo, None); + } = http_options(&repo, None, "https://example.com/does/not/matter"); assert_eq!( extra_headers, &["ExtraHeader: value2", "ExtraHeader: value3"], @@ -66,20 +70,36 @@ mod http { proxy_authenticate.is_none(), "no username means no authentication required" ); - assert_eq!(proxy_auth_method, ProxyAuthMethod::Basic, "TODO: implement auth"); + assert_eq!(proxy_auth_method, ProxyAuthMethod::Basic); assert_eq!(user_agent.as_deref(), Some("agentJustForHttp")); assert_eq!(connect_timeout, Some(std::time::Duration::from_millis(60 * 1024))); + assert_eq!(no_proxy, None); + assert!(!verbose, "verbose is disabled by default"); assert!( backend.is_none(), "backed is never set as it's backend specific, rather custom options typically" ) } + #[test] + fn http_verbose() { + let repo = repo("http-verbose"); + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); + assert!(opts.verbose); + } + + #[test] + fn http_no_proxy() { + let repo = repo("http-no-proxy"); + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); + assert_eq!(opts.no_proxy.as_deref(), Some("no validation done here")); + } + #[test] fn http_proxy_with_username() { let repo = repo("http-proxy-authenticated"); - let opts = http_options(&repo, None); + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); assert_eq!( opts.proxy.as_deref(), Some("http://user@localhost:9090"), @@ -100,7 +120,7 @@ mod http { fn empty_proxy_string_turns_it_off() { let repo = repo("http-proxy-empty"); - let opts = http_options(&repo, None); + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); assert_eq!( opts.proxy.as_deref(), Some(""), @@ -109,11 +129,117 @@ mod http { assert_eq!(opts.follow_redirects, FollowRedirects::None); } + #[test] + fn https_specific_proxy_only() { + let repo = repo("https-proxy-only"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://https"), + "if there is no other proxy setting, it will be used for https" + ); + + let opts = http_options(&repo, None, "http://host.local/repo"); + assert_eq!(opts.proxy, None, "non-https urls don't use this proxy at all"); + } + + #[test] + fn env_http_proxy_only() { + let repo = repo("gitoxide-http-proxy-only"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://http-fallback"), + "the `http_proxy` env var derived value serves as fallback…" + ); + + let opts = http_options(&repo, None, "http://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://http-fallback"), + "…for http-urls as well, as there is nothing else set." + ); + } + + #[test] + fn all_proxy_only() { + let repo = repo("gitoxide-all-proxy-only"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://all-proxy-fallback"), + "the `all_proxy` env var derived value serves as fallback…" + ); + + let opts = http_options(&repo, None, "http://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://all-proxy-fallback"), + "…for http-urls as well, as there is nothing else set." + ); + } + + #[test] + fn all_proxy_is_fallback() { + let repo = repo("gitoxide-all-proxy"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!(opts.proxy.as_deref(), Some("http://http")); + + let opts = http_options(&repo, None, "http://host.local/repo"); + assert_eq!(opts.proxy.as_deref(), Some("http://http")); + } + + #[test] + fn env_http_proxy_is_fallback() { + let repo = repo("gitoxide-http-proxy"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!(opts.proxy.as_deref(), Some("http://http")); + + let opts = http_options(&repo, None, "http://host.local/repo"); + assert_eq!(opts.proxy.as_deref(), Some("http://http")); + } + + #[test] + fn https_specific_proxy_is_only_a_fallback() { + let repo = repo("https-proxy"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://http"), + "if the http proxy is set, it will be used even for https as the latter is only a fallback (by env vars)" + ); + + let opts = http_options(&repo, None, "http://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some("http://http"), + "http urls use the http proxy configuration like normal" + ); + } + + #[test] + fn https_specific_proxy_empty() { + let repo = repo("https-proxy-empty"); + + let opts = http_options(&repo, None, "https://host.local/repo"); + assert_eq!( + opts.proxy.as_deref(), + Some(""), + "empty strings work just like they do for http.proxy (and empty strings indicate to unset it)" + ); + } + #[test] fn proxy_without_protocol_is_defaulted_to_http() { let repo = repo("http-proxy-auto-prefix"); - let opts = http_options(&repo, None); + let opts = http_options(&repo, None, "https://example.com/does/not/matter"); assert_eq!(opts.proxy.as_deref(), Some("http://localhost:9090")); assert_eq!(opts.follow_redirects, FollowRedirects::Initial); } diff --git a/git-repository/tests/repository/mod.rs b/git-repository/tests/repository/mod.rs index 310fcfdd02f..05f746849dc 100644 --- a/git-repository/tests/repository/mod.rs +++ b/git-repository/tests/repository/mod.rs @@ -11,7 +11,7 @@ mod worktree; #[test] fn size_in_memory() { let actual_size = std::mem::size_of::(); - let limit = 940; + let limit = 1000; assert!( actual_size <= limit, "size of Repository shouldn't change without us noticing, it's meant to be cloned: should have been below {:?}, was {} (bigger on windows)", diff --git a/git-repository/tests/repository/open.rs b/git-repository/tests/repository/open.rs index 2c74188c52a..3c29f87c5e9 100644 --- a/git-repository/tests/repository/open.rs +++ b/git-repository/tests/repository/open.rs @@ -44,3 +44,130 @@ mod submodules { .to_thread_local()) } } + +mod with_overrides { + use crate::util::named_subrepo_opts; + use git_object::bstr::BStr; + use git_repository as git; + use git_sec::Permission; + use git_testtools::Env; + use serial_test::serial; + use std::borrow::Cow; + + #[test] + #[serial] + fn order_from_api_and_cli_and_environment() -> crate::Result { + let _env = Env::new() + .set("GIT_HTTP_USER_AGENT", "agent-from-env") + .set("GIT_HTTP_LOW_SPEED_LIMIT", "1") + .set("GIT_HTTP_LOW_SPEED_TIME", "1") + .set("GIT_HTTP_PROXY_AUTHMETHOD", "negotiate") + .set("GIT_CURL_VERBOSE", "true") + .set("https_proxy", "https-lower-override") + .set("HTTPS_PROXY", "https-upper") + .set("http_proxy", "http-lower") + .set("all_proxy", "all-proxy-lower") + .set("ALL_PROXY", "all-proxy") + .set("no_proxy", "no-proxy-lower") + .set("NO_PROXY", "no-proxy") + .set("GIT_PROTOCOL_FROM_USER", "file-allowed"); + let mut opts = git::open::Options::isolated() + .config_overrides([ + "http.userAgent=agent-from-api", + "http.lowSpeedLimit=2", + "http.lowSpeedTime=2", + ]) + .cli_overrides([ + "http.userAgent=agent-from-cli", + "http.lowSpeedLimit=3", + "http.lowSpeedTime=3", + ]); + opts.permissions.env.git_prefix = Permission::Allow; + opts.permissions.env.http_transport = Permission::Allow; + let repo = named_subrepo_opts("make_config_repos.sh", "http-config", opts)?; + let config = repo.config_snapshot(); + assert_eq!( + config.strings_by_key("http.userAgent").expect("at least one value"), + [ + cow_bstr("agentJustForHttp"), + cow_bstr("agent-from-cli"), + cow_bstr("agent-from-api"), + cow_bstr("agent-from-env") + ] + ); + assert_eq!( + config + .integers_by_key("http.lowSpeedLimit") + .transpose()? + .expect("many values"), + [5120, 3, 2, 1] + ); + assert_eq!( + config + .integers_by_key("http.lowSpeedTime") + .transpose()? + .expect("many values"), + [10, 3, 2, 1] + ); + assert_eq!( + config + .strings_by_key("http.proxyAuthMethod") + .expect("at least one value"), + [cow_bstr("basic"), cow_bstr("negotiate"),] + ); + assert_eq!( + config + .strings_by_key("gitoxide.https.proxy") + .expect("at least one value"), + [ + cow_bstr("https-upper"), + cow_bstr(if cfg!(windows) { + "https-upper" // on windows, environment variables are case-insensitive + } else { + "https-lower-override" + }) + ] + ); + assert_eq!( + config + .strings_by_key("gitoxide.http.proxy") + .expect("at least one value"), + [cow_bstr("http-lower")] + ); + assert_eq!( + config + .strings_by_key("gitoxide.http.allProxy") + .expect("at least one value"), + [ + cow_bstr("all-proxy"), // on windows, environment variables are case-insensitive + cow_bstr(if cfg!(windows) { "all-proxy" } else { "all-proxy-lower" }) + ] + ); + assert_eq!( + config + .strings_by_key("gitoxide.http.noProxy") + .expect("at least one value"), + [ + cow_bstr("no-proxy"), // on windows, environment variables are case-insensitive + cow_bstr(if cfg!(windows) { "no-proxy" } else { "no-proxy-lower" }) + ] + ); + assert_eq!( + config + .strings_by_key("gitoxide.http.verbose") + .expect("at least one value"), + [cow_bstr("true")] + ); + assert_eq!( + config + .strings_by_key("gitoxide.allow.protocolFromUser") + .expect("at least one value"), + [cow_bstr("file-allowed")] + ); + Ok(()) + } + + fn cow_bstr(s: &str) -> Cow { + Cow::Borrowed(s.into()) + } +} diff --git a/git-repository/tests/util/mod.rs b/git-repository/tests/util/mod.rs index 8058f3e0032..f2aac4176de 100644 --- a/git-repository/tests/util/mod.rs +++ b/git-repository/tests/util/mod.rs @@ -23,6 +23,11 @@ pub fn named_repo(name: &str) -> Result { Ok(ThreadSafeRepository::open_opts(repo_path, restricted())?.to_thread_local()) } +pub fn named_subrepo_opts(fixture: &str, name: &str, opts: open::Options) -> Result { + let repo_path = git_testtools::scripted_fixture_repo_read_only(fixture)?.join(name); + Ok(ThreadSafeRepository::open_opts(repo_path, opts)?.to_thread_local()) +} + pub fn restricted() -> open::Options { open::Options::isolated() } diff --git a/git-sec/src/permission.rs b/git-sec/src/permission.rs index b43e6e69aa2..5bd5f2c325f 100644 --- a/git-sec/src/permission.rs +++ b/git-sec/src/permission.rs @@ -44,7 +44,7 @@ impl Permission { } /// Like [`check()`][Self::check()], but degenerates the type to an option to make it more useful in cases where - /// `Forbid` shoudn't abort the entire operation. + /// `Forbid` shouldn't abort the entire operation. pub fn check_opt(&self, resource: R) -> Option { match self { Permission::Allow => Some(resource), diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index cfa68cd997e..0999c6bcaa5 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -729,14 +729,43 @@ static GIT_CONFIG: &[Record] = &[ Record { config: "gitoxide.userAgent", usage: InModule { - name: "remote::connection", - deviation: None + name: "config::cache", + deviation: Some("The user agent presented on the git protocol layer, serving as fallback for when no http.userAgent is set.") + } + }, + Record { + config: "gitoxide.https.proxy", + usage: InModule { + name: "repository::config::transport", + deviation: Some("Used only if the url to access is https, created from 'HTTPS_PROXY' and 'https_proxy' env-vars") + } + }, + Record { + config: "gitoxide.http.proxy", + usage: InModule { + name: "repository::config::transport", + deviation: Some("created from 'http_proxy' env-var.") + } + }, + Record { + config: "gitoxide.http.allProxy", + usage: InModule { + name: "repository::config::transport", + deviation: Some("created from 'all_proxy' or 'ALL_PROXY' env-var.") + } + }, + Record { + config: "gitoxide.http.verbose", + usage: InModule { + name: "repository::config::transport", + deviation: Some("created from 'GIT_CURL_VERBOSE' to print debug output to stderr.") } }, Record { config: "gitoxide.http.noProxy", - usage: NotPlanned { - reason: "on demand, without it it's not possible to implement environment overrides via `no_proxy` or `NO_PROXY` for a list of hostnames or `*`" + usage: InModule { + name: "repository::config::transport", + deviation: Some("created from 'no_proxy' or 'NO_PROXY' env-var.") } }, Record { @@ -745,7 +774,14 @@ static GIT_CONFIG: &[Record] = &[ name: "repository::config::transport", deviation: Some("entirely new, and in milliseconds like all other timeout suffixed variables in the git config") } - } + }, + Record { + config: "gitoxide.allow.protocolFromUser", + usage: InModule { + name: "remote::url::scheme_permission", + deviation: Some("corresponds to GIT_PROTOCOL_FROM_USER environment variable") + } + }, ]; /// A programmatic way to record and display progress. From 49f39d6bb487c0254176a5082f2c7851b83952a1 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 16:10:58 +0100 Subject: [PATCH 26/29] change!: `open::ReplacementObjects` is removed in favor of two custom git-configuration flags. Now it's possible to map the environment variables `GIT_REPLACE_REF_BASE` and `GIT_NO_REPLACE_OBJECTS` to custom git configuration keys which can also be set, namely `gitoxide.odb.replaceObjectsRefBase` and `gitoxide.odb.noReplaceObjects`. Along with the possibility of disabling the usage of `GIT_` prefixed environment variables one reaches the previous level of control without making object replacement a special case. --- git-repository/src/config/cache/init.rs | 24 +++++++++ git-repository/src/config/mod.rs | 5 ++ git-repository/src/open/mod.rs | 65 +++---------------------- git-repository/src/open/options.rs | 12 +---- git-repository/src/open/repository.rs | 34 +++++++++++-- git-repository/tests/repository/open.rs | 18 ++++++- src/plumbing/progress.rs | 14 ++++++ 7 files changed, 99 insertions(+), 73 deletions(-) diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index aa34c4c2a05..6df1791d7e8 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -319,6 +319,30 @@ fn apply_environment_overrides( } } + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("objects".into()))) + .expect("statically known valid section name"); + + for (var, key) in [ + ("GIT_NO_REPLACE_OBJECTS", "noReplace"), + ("GIT_REPLACE_REF_BASE", "replaceRefBase"), + ] { + if let Some(value) = var_as_bstring(var, http_transport) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + { let mut section = env_override .new_section("gitoxide", Some(Cow::Borrowed("http".into()))) diff --git a/git-repository/src/config/mod.rs b/git-repository/src/config/mod.rs index 4e6d780e054..d752393235c 100644 --- a/git-repository/src/config/mod.rs +++ b/git-repository/src/config/mod.rs @@ -51,6 +51,11 @@ pub(crate) mod section { pub enum Error { #[error("Could not read configuration file")] Io(#[from] std::io::Error), + #[error("Could not decode configuration value at {key:?}")] + Value { + source: git_config::value::Error, + key: &'static str, + }, #[error(transparent)] Init(#[from] git_config::file::init::Error), #[error(transparent)] diff --git a/git-repository/src/open/mod.rs b/git-repository/src/open/mod.rs index 2150bda21ef..53f73bfa167 100644 --- a/git-repository/src/open/mod.rs +++ b/git-repository/src/open/mod.rs @@ -2,66 +2,17 @@ use std::path::PathBuf; use crate::{bstr::BString, config, permission, Permissions}; -/// A way to configure the usage of replacement objects, see `git replace`. -#[derive(Debug, Clone)] -pub enum ReplacementObjects { - /// Allow replacement objects and configure the ref prefix the standard environment variable `GIT_REPLACE_REF_BASE`, - /// or default to the standard `refs/replace/` prefix. - UseWithEnvironmentRefPrefixOrDefault { - /// If true, default true, a standard environment variable `GIT_NO_REPLACE_OBJECTS` to disable replacement objects entirely. - allow_disable_via_environment: bool, - }, - /// Use replacement objects and configure the prefix yourself. - UseWithRefPrefix { - /// The ref prefix to use, like `refs/alternative/` - note the trailing slash. - prefix: PathBuf, - /// If true, default true, a standard environment variable `GIT_NO_REPLACE_OBJECTS` - allow_disable_via_environment: bool, - }, - /// Do not use replacement objects at all. - Disable, -} - -impl Default for ReplacementObjects { - fn default() -> Self { - ReplacementObjects::UseWithEnvironmentRefPrefixOrDefault { - allow_disable_via_environment: true, - } - } -} - -impl ReplacementObjects { - fn refs_prefix(self) -> Option { - use ReplacementObjects::*; - let is_disabled = |allow_env: bool| allow_env && std::env::var_os("GIT_NO_REPLACE_OBJECTS").is_some(); - match self { - UseWithEnvironmentRefPrefixOrDefault { - allow_disable_via_environment, - } => { - if is_disabled(allow_disable_via_environment) { - return None; - }; - PathBuf::from(std::env::var("GIT_REPLACE_REF_BASE").unwrap_or_else(|_| "refs/replace/".into())).into() - } - UseWithRefPrefix { - prefix, - allow_disable_via_environment, - } => { - if is_disabled(allow_disable_via_environment) { - return None; - }; - prefix.into() - } - Disable => None, - } - } -} - -/// The options used in [`ThreadSafeRepository::open_opts`] +/// The options used in [`ThreadSafeRepository::open_opts()`][crate::ThreadSafeRepository::open_opts()]. +/// +/// ### Replacement Objects for the object database +/// +/// The environment variables `GIT_REPLACE_REF_BASE` and `GIT_NO_REPLACE_OBJECTS` are mapped to `gitoxide.objects.replaceRefBase` +/// and `gitoxide.objects.noReplace` respectively and then interpreted exactly as their environment variable counterparts. +/// +/// Use [Permissions] to control which environment variables can be read, and config-overrides to control these values programmatically. #[derive(Clone)] pub struct Options { pub(crate) object_store_slots: git_odb::store::init::Slots, - pub(crate) replacement_objects: ReplacementObjects, /// Define what is allowed while opening a repository. pub permissions: Permissions, pub(crate) git_dir_trust: Option, diff --git a/git-repository/src/open/options.rs b/git-repository/src/open/options.rs index 3ecee14227b..df9f84df6c0 100644 --- a/git-repository/src/open/options.rs +++ b/git-repository/src/open/options.rs @@ -1,4 +1,4 @@ -use super::{Error, Options, ReplacementObjects}; +use super::{Error, Options}; use crate::bstr::BString; use crate::{config, Permissions, ThreadSafeRepository}; use std::path::PathBuf; @@ -7,7 +7,6 @@ impl Default for Options { fn default() -> Self { Options { object_store_slots: Default::default(), - replacement_objects: Default::default(), permissions: Default::default(), git_dir_trust: None, filter_config_section: None, @@ -63,13 +62,6 @@ impl Options { self } - // TODO: tests - /// Configure replacement objects, see the [`ReplacementObjects`] type for details. - pub fn replacement_objects(mut self, config: ReplacementObjects) -> Self { - self.replacement_objects = config; - self - } - // TODO: tests /// Set the given permissions, which are typically derived by a `Trust` level. pub fn permissions(mut self, permissions: Permissions) -> Self { @@ -148,7 +140,6 @@ impl git_sec::trust::DefaultForLevel for Options { match level { git_sec::Trust::Full => Options { object_store_slots: Default::default(), - replacement_objects: Default::default(), permissions: Permissions::default_for_level(level), git_dir_trust: git_sec::Trust::Full.into(), filter_config_section: Some(config::section::is_trusted), @@ -161,7 +152,6 @@ impl git_sec::trust::DefaultForLevel for Options { }, git_sec::Trust::Reduced => Options { object_store_slots: git_odb::store::init::Slots::Given(32), // limit resource usage - replacement_objects: ReplacementObjects::Disable, // don't be tricked into seeing manufactured objects permissions: Permissions::default_for_level(level), git_dir_trust: git_sec::Trust::Reduced.into(), filter_config_section: Some(config::section::is_trusted), diff --git a/git-repository/src/open/repository.rs b/git-repository/src/open/repository.rs index 7ea5c885cf0..65fe60fd1b5 100644 --- a/git-repository/src/open/repository.rs +++ b/git-repository/src/open/repository.rs @@ -1,7 +1,9 @@ use super::{Error, Options}; use crate::config::cache::interpolate_context; +use crate::config::cache::util::ApplyLeniency; use crate::{config, permission, Permissions, ThreadSafeRepository}; use git_features::threading::OwnShared; +use std::borrow::Cow; use std::path::PathBuf; #[derive(Default, Clone)] @@ -108,7 +110,6 @@ impl ThreadSafeRepository { git_dir_trust, object_store_slots, filter_config_section, - ref replacement_objects, lossy_config, lenient_config, bail_if_untrusted, @@ -191,9 +192,7 @@ impl ThreadSafeRepository { } refs.write_reflog = config::cache::util::reflog_or_default(config.reflog, worktree_dir.is_some()); - let replacements = replacement_objects - .clone() - .refs_prefix() + let replacements = replacement_objects_refs_prefix(&config.resolved, lenient_config, filter_config_section)? .and_then(|prefix| { let platform = refs.iter().ok()?; let iter = platform.prefixed(&prefix).ok()?; @@ -233,6 +232,33 @@ impl ThreadSafeRepository { } } +// TODO: tests +fn replacement_objects_refs_prefix( + config: &git_config::File<'static>, + lenient: bool, + mut filter_config_section: fn(&git_config::file::Metadata) -> bool, +) -> Result, Error> { + let key = "gitoxide.objects.noReplace"; + let is_disabled = config + .boolean_filter_by_key(key, &mut filter_config_section) + .transpose() + .with_leniency(lenient) + .map_err(|err| config::Error::Value { source: err, key })? + .unwrap_or_default(); + + if is_disabled { + return Ok(None); + } + + let ref_base = git_path::from_bstr( + config + .string_filter_by_key("gitoxide.objects.replaceRefBase", &mut filter_config_section) + .unwrap_or_else(|| Cow::Borrowed("refs/replace/".into())), + ) + .into_owned(); + Ok(ref_base.into()) +} + fn check_safe_directories( git_dir: &std::path::Path, git_install_dir: Option<&std::path::Path>, diff --git a/git-repository/tests/repository/open.rs b/git-repository/tests/repository/open.rs index 3c29f87c5e9..af3511de776 100644 --- a/git-repository/tests/repository/open.rs +++ b/git-repository/tests/repository/open.rs @@ -70,7 +70,9 @@ mod with_overrides { .set("ALL_PROXY", "all-proxy") .set("no_proxy", "no-proxy-lower") .set("NO_PROXY", "no-proxy") - .set("GIT_PROTOCOL_FROM_USER", "file-allowed"); + .set("GIT_PROTOCOL_FROM_USER", "file-allowed") + .set("GIT_REPLACE_REF_BASE", "refs/replace-mine") + .set("GIT_NO_REPLACE_OBJECTS", "no-replace"); let mut opts = git::open::Options::isolated() .config_overrides([ "http.userAgent=agent-from-api", @@ -164,6 +166,20 @@ mod with_overrides { .expect("at least one value"), [cow_bstr("file-allowed")] ); + assert_eq!( + config + .string_by_key("gitoxide.objects.noReplace") + .expect("at least one value") + .as_ref(), + "no-replace" + ); + assert_eq!( + config + .string_by_key("gitoxide.objects.replaceRefBase") + .expect("at least one value") + .as_ref(), + "refs/replace-mine" + ); Ok(()) } diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index 0999c6bcaa5..9ccdef780de 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -782,6 +782,20 @@ static GIT_CONFIG: &[Record] = &[ deviation: Some("corresponds to GIT_PROTOCOL_FROM_USER environment variable") } }, + Record { + config: "gitoxide.objects.replaceRefBase", + usage: InModule { + name: "open", + deviation: Some("corresponds to the GIT_REPLACE_REF_BASE environment variable") + } + }, + Record { + config: "gitoxide.objects.noReplace", + usage: InModule { + name: "open", + deviation: Some("corresponds to the GIT_NO_REPLACE_OBJECTS environment variable") + } + }, ]; /// A programmatic way to record and display progress. From a4ac9cf3e667a3059e33aac8188150529578622d Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 17:09:11 +0100 Subject: [PATCH 27/29] change: represent `GIT_(COMMITTER|AUTHOR)_(NAME|EMAIL|DATE)` with git configuration. That way it becomes more obvious where values are coming from. --- git-repository/src/config/cache/access.rs | 2 +- git-repository/src/config/cache/init.rs | 102 +++++++++++++++++- git-repository/src/config/mod.rs | 2 - git-repository/src/repository/identity.rs | 69 ++++++------ git-repository/src/repository/permissions.rs | 9 +- .../tests/repository/config/identity.rs | 4 + git-repository/tests/repository/open.rs | 61 ++++++----- src/plumbing/progress.rs | 57 +++++++++- 8 files changed, 233 insertions(+), 73 deletions(-) diff --git a/git-repository/src/config/cache/access.rs b/git-repository/src/config/cache/access.rs index 41017e20233..d3504e9b820 100644 --- a/git-repository/src/config/cache/access.rs +++ b/git-repository/src/config/cache/access.rs @@ -60,7 +60,7 @@ impl Cache { pub(crate) fn personas(&self) -> &identity::Personas { self.personas - .get_or_init(|| identity::Personas::from_config_and_env(&self.resolved, self.git_prefix)) + .get_or_init(|| identity::Personas::from_config_and_env(&self.resolved)) } pub(crate) fn url_rewrite(&self) -> &remote::url::Rewrite { diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index 6df1791d7e8..996a9ad263c 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -31,6 +31,7 @@ impl Cache { xdg_config_home: xdg_config_home_env, ssh_prefix: _, http_transport, + identity, }: repository::permissions::Environment, repository::permissions::Config { git_binary: use_installation, @@ -132,7 +133,7 @@ impl Cache { source: git_config::Source::Api, })?; } - apply_environment_overrides(&mut globals, *git_prefix, http_transport)?; + apply_environment_overrides(&mut globals, *git_prefix, http_transport, identity)?; globals }; @@ -163,7 +164,6 @@ impl Cache { #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] url_scheme: Default::default(), diff_algorithm: Default::default(), - git_prefix, }) } @@ -243,6 +243,7 @@ fn apply_environment_overrides( config: &mut File<'static>, git_prefix: Permission, http_transport: Permission, + identity: Permission, ) -> Result<(), Error> { fn var_as_bstring(var: &str, perm: Permission) -> Option { perm.check_opt(var) @@ -298,13 +299,106 @@ fn apply_environment_overrides( } } + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("committer".into()))) + .expect("statically known valid section name"); + + for (var, key) in [ + ("GIT_COMMITTER_NAME", "nameFallback"), + ("GIT_COMMITTER_EMAIL", "emailFallback"), + ] { + if let Some(value) = var_as_bstring(var, git_prefix) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("author".into()))) + .expect("statically known valid section name"); + + for (var, key) in [ + ("GIT_AUTHOR_NAME", "nameFallback"), + ("GIT_AUTHOR_EMAIL", "emailFallback"), + ] { + if let Some(value) = var_as_bstring(var, git_prefix) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("commit".into()))) + .expect("statically known valid section name"); + + for (var, key) in [ + ("GIT_COMMITTER_DATE", "committerDate"), + ("GIT_AUTHOR_DATE", "authorDate"), + ] { + if let Some(value) = var_as_bstring(var, git_prefix) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + { let mut section = env_override .new_section("gitoxide", Some(Cow::Borrowed("allow".into()))) .expect("statically known valid section name"); for (var, key) in [("GIT_PROTOCOL_FROM_USER", "protocolFromUser")] { - if let Some(value) = var_as_bstring(var, http_transport) { + if let Some(value) = var_as_bstring(var, git_prefix) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("gitoxide", Some(Cow::Borrowed("user".into()))) + .expect("statically known valid section name"); + + for (var, key) in [("EMAIL", "emailFallback")] { + if let Some(value) = var_as_bstring(var, identity) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), @@ -328,7 +422,7 @@ fn apply_environment_overrides( ("GIT_NO_REPLACE_OBJECTS", "noReplace"), ("GIT_REPLACE_REF_BASE", "replaceRefBase"), ] { - if let Some(value) = var_as_bstring(var, http_transport) { + if let Some(value) = var_as_bstring(var, git_prefix) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), diff --git a/git-repository/src/config/mod.rs b/git-repository/src/config/mod.rs index d752393235c..7a9ec4ccf9d 100644 --- a/git-repository/src/config/mod.rs +++ b/git-repository/src/config/mod.rs @@ -208,7 +208,5 @@ pub(crate) struct Cache { xdg_config_home_env: git_sec::Permission, /// Define how we can use values obtained with `xdg_config(…)`. and its `HOME` variable. home_env: git_sec::Permission, - /// How to use git-prefixed environment variables - git_prefix: git_sec::Permission, // TODO: make core.precomposeUnicode available as well. } diff --git a/git-repository/src/repository/identity.rs b/git-repository/src/repository/identity.rs index 18bba3061a9..35778cb9c33 100644 --- a/git-repository/src/repository/identity.rs +++ b/git-repository/src/repository/identity.rs @@ -1,6 +1,6 @@ -use std::{borrow::Cow, time::SystemTime}; +use std::time::SystemTime; -use crate::bstr::BString; +use crate::bstr::{BString, ByteSlice}; /// Identity handling. impl crate::Repository { @@ -105,43 +105,44 @@ pub(crate) struct Personas { } impl Personas { - pub fn from_config_and_env(config: &git_config::File<'_>, git_env: git_sec::Permission) -> Self { - fn env_var(name: &str) -> Option { - std::env::var_os(name).map(|value| git_path::into_bstr(Cow::Owned(value.into())).into_owned()) - } - fn entity_in_section(name: &str, config: &git_config::File<'_>) -> (Option, Option) { - config + pub fn from_config_and_env(config: &git_config::File<'_>) -> Self { + fn entity_in_section( + name: &str, + config: &git_config::File<'_>, + fallback: bool, + ) -> (Option, Option) { + let fallback = fallback + .then(|| config.section("gitoxide", Some(name.into())).ok()) + .flatten(); + let (name, email) = config .section(name, None) - .map(|section| { - ( - section.value("name").map(|v| v.into_owned()), - section.value("email").map(|v| v.into_owned()), - ) - }) - .unwrap_or_default() + .map(|section| (section.value("name"), section.value("email"))) + .unwrap_or_default(); + ( + name.or_else(|| fallback.as_ref().and_then(|s| s.value("nameFallback"))) + .map(|v| v.into_owned()), + email + .or_else(|| fallback.as_ref().and_then(|s| s.value("emailFallback"))) + .map(|v| v.into_owned()), + ) } + let now = SystemTime::now(); + let parse_date = |key: &str| -> Option { + config.string_by_key(key).and_then(|date| { + date.to_str() + .ok() + .and_then(|date| git_date::parse(date, Some(now)).ok()) + }) + }; - let (mut committer_name, mut committer_email) = entity_in_section("committer", config); - let mut committer_date = None; - let (mut author_name, mut author_email) = entity_in_section("author", config); - let mut author_date = None; - let (user_name, mut user_email) = entity_in_section("user", config); - - if git_env.eq(&git_sec::Permission::Allow) { - committer_name = committer_name.or_else(|| env_var("GIT_COMMITTER_NAME")); - committer_email = committer_email.or_else(|| env_var("GIT_COMMITTER_EMAIL")); - committer_date = std::env::var("GIT_COMMITTER_DATE") - .ok() - .and_then(|date| git_date::parse(&date, Some(SystemTime::now())).ok()); + let (committer_name, committer_email) = entity_in_section("committer", config, true); + let (author_name, author_email) = entity_in_section("author", config, true); + let (user_name, mut user_email) = entity_in_section("user", config, false); - author_name = author_name.or_else(|| env_var("GIT_AUTHOR_NAME")); - author_email = author_email.or_else(|| env_var("GIT_AUTHOR_EMAIL")); - author_date = std::env::var("GIT_AUTHOR_DATE") - .ok() - .and_then(|date| git_date::parse(&date, Some(SystemTime::now())).ok()); + let committer_date = parse_date("gitoxide.commit.committerDate"); + let author_date = parse_date("gitoxide.commit.authorDate"); - user_email = user_email.or_else(|| env_var("EMAIL")); // NOTE: we don't have permission for this specific one… - } + user_email = user_email.or_else(|| config.string_by_key("gitoxide.user.email").map(|v| v.into_owned())); Personas { user: Entity { name: user_name, diff --git a/git-repository/src/repository/permissions.rs b/git-repository/src/repository/permissions.rs index b4ac124eafe..a44a06ba3be 100644 --- a/git-repository/src/repository/permissions.rs +++ b/git-repository/src/repository/permissions.rs @@ -74,9 +74,14 @@ pub struct Environment { pub ssh_prefix: git_sec::Permission, /// Control if environment variables to configure the HTTP transport, like `http_proxy` may be used. /// - /// Note that those http-transport related environment variables prefixed with `GIT_` are falling under the + /// Note that http-transport related environment variables prefixed with `GIT_` are falling under the /// `git_prefix` permission, like `GIT_HTTP_USER_AGENT`. pub http_transport: git_sec::Permission, + /// Control if the `EMAIL` environment variables may be read. + /// + /// Note that identity related environment variables prefixed with `GIT_` are falling under the + /// `git_prefix` permission, like `GIT_AUTHOR_NAME`. + pub identity: git_sec::Permission, } impl Environment { @@ -88,6 +93,7 @@ impl Environment { git_prefix: git_sec::Permission::Allow, ssh_prefix: git_sec::Permission::Allow, http_transport: git_sec::Permission::Allow, + identity: git_sec::Permission::Allow, } } } @@ -133,6 +139,7 @@ impl Permissions { ssh_prefix: deny, git_prefix: deny, http_transport: deny, + identity: deny, } }, } diff --git a/git-repository/tests/repository/config/identity.rs b/git-repository/tests/repository/config/identity.rs index f806da7fd03..e10da8639d1 100644 --- a/git-repository/tests/repository/config/identity.rs +++ b/git-repository/tests/repository/config/identity.rs @@ -21,6 +21,10 @@ fn author_and_committer_and_fallback() { .set("GIT_AUTHOR_NAME", "author") .set("GIT_AUTHOR_EMAIL", "author@email") .set("GIT_AUTHOR_DATE", "1979-02-26 18:30:00") + .set("GIT_COMMITTER_NAME", "commiter-overrider-unused") + .set("GIT_COMMITTER_EMAIL", "committer-override-unused@email") + .set("GIT_COMMITTER_DATE", "1980-02-26 18:30:00") + .set("EMAIL", "general@email-unused") .set("GIT_CONFIG_COUNT", "1") .set("GIT_CONFIG_KEY_0", "include.path") .set("GIT_CONFIG_VALUE_0", work_dir.join("c.config").display().to_string()); diff --git a/git-repository/tests/repository/open.rs b/git-repository/tests/repository/open.rs index af3511de776..49689b4d433 100644 --- a/git-repository/tests/repository/open.rs +++ b/git-repository/tests/repository/open.rs @@ -57,6 +57,7 @@ mod with_overrides { #[test] #[serial] fn order_from_api_and_cli_and_environment() -> crate::Result { + let default_date = "1979-02-26 18:30:00"; let _env = Env::new() .set("GIT_HTTP_USER_AGENT", "agent-from-env") .set("GIT_HTTP_LOW_SPEED_LIMIT", "1") @@ -72,7 +73,14 @@ mod with_overrides { .set("NO_PROXY", "no-proxy") .set("GIT_PROTOCOL_FROM_USER", "file-allowed") .set("GIT_REPLACE_REF_BASE", "refs/replace-mine") - .set("GIT_NO_REPLACE_OBJECTS", "no-replace"); + .set("GIT_NO_REPLACE_OBJECTS", "no-replace") + .set("GIT_COMMITTER_NAME", "committer name") + .set("GIT_COMMITTER_EMAIL", "committer email") + .set("GIT_COMMITTER_DATE", default_date) + .set("GIT_AUTHOR_NAME", "author name") + .set("GIT_AUTHOR_EMAIL", "author email") + .set("GIT_AUTHOR_DATE", default_date) + .set("EMAIL", "user email"); let mut opts = git::open::Options::isolated() .config_overrides([ "http.userAgent=agent-from-api", @@ -86,6 +94,7 @@ mod with_overrides { ]); opts.permissions.env.git_prefix = Permission::Allow; opts.permissions.env.http_transport = Permission::Allow; + opts.permissions.env.identity = Permission::Allow; let repo = named_subrepo_opts("make_config_repos.sh", "http-config", opts)?; let config = repo.config_snapshot(); assert_eq!( @@ -154,32 +163,30 @@ mod with_overrides { cow_bstr(if cfg!(windows) { "no-proxy" } else { "no-proxy-lower" }) ] ); - assert_eq!( - config - .strings_by_key("gitoxide.http.verbose") - .expect("at least one value"), - [cow_bstr("true")] - ); - assert_eq!( - config - .strings_by_key("gitoxide.allow.protocolFromUser") - .expect("at least one value"), - [cow_bstr("file-allowed")] - ); - assert_eq!( - config - .string_by_key("gitoxide.objects.noReplace") - .expect("at least one value") - .as_ref(), - "no-replace" - ); - assert_eq!( - config - .string_by_key("gitoxide.objects.replaceRefBase") - .expect("at least one value") - .as_ref(), - "refs/replace-mine" - ); + for (key, expected) in [ + ("gitoxide.http.verbose", "true"), + ("gitoxide.allow.protocolFromUser", "file-allowed"), + ("gitoxide.objects.noReplace", "no-replace"), + ("gitoxide.objects.replaceRefBase", "refs/replace-mine"), + ("gitoxide.committer.nameFallback", "committer name"), + ("gitoxide.committer.emailFallback", "committer email"), + ("gitoxide.author.nameFallback", "author name"), + ("gitoxide.author.emailFallback", "author email"), + ("gitoxide.commit.authorDate", default_date), + ("gitoxide.commit.committerDate", default_date), + ("gitoxide.user.emailFallback", "user email"), + ] { + assert_eq!( + config + .string_by_key(key) + .unwrap_or_else(|| panic!("no value for {key}")) + .as_ref(), + expected, + "{} == {}", + key, + expected + ); + } Ok(()) } diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index 9ccdef780de..c78086de356 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -348,28 +348,28 @@ static GIT_CONFIG: &[Record] = &[ config: "committer.name", usage: InModule { name: "repository::identity", - deviation: None, + deviation: Some("overridden by 'GIT_COMMITTER_NAME'"), }, }, Record { config: "committer.email", usage: InModule { name: "repository::identity", - deviation: None, + deviation: Some("overridden by 'GIT_COMMITTER_EMAIL'"), }, }, Record { config: "author.name", usage: InModule { name: "repository::identity", - deviation: None, + deviation: Some("overridden by 'GIT_AUTHOR_NAME'"), }, }, Record { config: "author.email", usage: InModule { name: "repository::identity", - deviation: None, + deviation: Some("overridden by 'GIT_AUTHOR_EMAIL'"), }, }, Record { @@ -796,6 +796,55 @@ static GIT_CONFIG: &[Record] = &[ deviation: Some("corresponds to the GIT_NO_REPLACE_OBJECTS environment variable") } }, + Record { + config: "gitoxide.commit.authorDate", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the GIT_AUTHOR_DATE environment variable") + } + }, + Record { + config: "gitoxide.commit.committerDate", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the GIT_COMMITTER_DATE environment variable") + } + }, + Record { + config: "gitoxide.author.nameFallback", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the GIT_AUTHOR_NAME environment variable and is a fallback for `author.name`") + } + }, + Record { + config: "gitoxide.author.emailFallback", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the GIT_AUTHOR_EMAIL environment variable and is a fallback for `author.email`") + } + }, + Record { + config: "gitoxide.committer.nameFallback", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the GIT_COMMITTER_NAME environment variable and is a fallback for `committer.name`") + } + }, + Record { + config: "gitoxide.committer.emailFallback", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the GIT_COMMITTER_EMAIL environment variable and is a fallback for `committer.email`") + } + }, + Record { + config: "gitoxide.user.emailFallback", + usage: InModule { + name: "repository::identity", + deviation: Some("corresponds to the EMAIL environment variable and is a fallback for `user.email`") + } + }, ]; /// A programmatic way to record and display progress. From becbd8d896a1663f1607be4e86e632773e926f1f Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 19:35:43 +0100 Subject: [PATCH 28/29] feat!: represent object cache configuration like `GITOXIDE_PACK_CACHE_MEMORY` in git-configuration. That way there is a unified system for how to set values, which may be overridable by configuration variables or not. With this changes, the explicit application of environment variables for setting the cache isn't required anymore as everything happens using git-configuration, and automatically, while providing full control like before. --- git-repository/examples/stats.rs | 2 +- git-repository/src/config/cache/init.rs | 40 ++++++++++-- git-repository/src/config/cache/util.rs | 25 ++++++++ git-repository/src/config/mod.rs | 5 ++ git-repository/src/lib.rs | 3 - git-repository/src/repository/cache.rs | 63 ------------------- git-repository/src/repository/init.rs | 37 ++++++++--- git-repository/src/repository/permissions.rs | 4 ++ .../make_config_repos.tar.xz | 4 +- .../tests/fixtures/make_config_repos.sh | 12 ++++ git-repository/tests/repository/open.rs | 30 ++++++++- src/plumbing/progress.rs | 12 +++- 12 files changed, 151 insertions(+), 86 deletions(-) diff --git a/git-repository/examples/stats.rs b/git-repository/examples/stats.rs index 6305a2585c4..3772e7b23ab 100644 --- a/git-repository/examples/stats.rs +++ b/git-repository/examples/stats.rs @@ -2,7 +2,7 @@ use git_repository as git; use git_repository::Reference; fn main() -> Result<(), Box> { - let mut repo = git::discover(".")?.apply_environment(); + let mut repo = git::discover(".")?; println!("Repo: {}", repo.work_dir().unwrap_or_else(|| repo.git_dir()).display()); let mut max_commit_size = 0; let mut avg_commit_size = 0; diff --git a/git-repository/src/config/cache/init.rs b/git-repository/src/config/cache/init.rs index 996a9ad263c..5c9f8d38b5d 100644 --- a/git-repository/src/config/cache/init.rs +++ b/git-repository/src/config/cache/init.rs @@ -32,6 +32,7 @@ impl Cache { ssh_prefix: _, http_transport, identity, + gitoxide_prefix, }: repository::permissions::Environment, repository::permissions::Config { git_binary: use_installation, @@ -133,7 +134,7 @@ impl Cache { source: git_config::Source::Api, })?; } - apply_environment_overrides(&mut globals, *git_prefix, http_transport, identity)?; + apply_environment_overrides(&mut globals, *git_prefix, http_transport, identity, gitoxide_prefix)?; globals }; @@ -144,12 +145,16 @@ impl Cache { let ignore_case = config_bool(&config, "core.ignoreCase", false, lenient_config)?; let use_multi_pack_index = config_bool(&config, "core.multiPackIndex", true, lenient_config)?; let object_kind_hint = util::disambiguate_hint(&config); + let (pack_cache_bytes, object_cache_bytes) = + util::parse_object_caches(&config, lenient_config, filter_config_section)?; // NOTE: When adding a new initial cache, consider adjusting `reread_values_and_clear_caches()` as well. Ok(Cache { resolved: config.into(), use_multi_pack_index, object_hash, object_kind_hint, + pack_cache_bytes, + object_cache_bytes, reflog, is_bare, ignore_case, @@ -203,6 +208,8 @@ impl Cache { self.personas = Default::default(); self.url_rewrite = Default::default(); self.diff_algorithm = Default::default(); + (self.pack_cache_bytes, self.object_cache_bytes) = + util::parse_object_caches(config, self.lenient_config, self.filter_config_section)?; #[cfg(any(feature = "blocking-network-client", feature = "async-network-client"))] { self.url_scheme = Default::default(); @@ -244,6 +251,7 @@ fn apply_environment_overrides( git_prefix: Permission, http_transport: Permission, identity: Permission, + gitoxide_prefix: Permission, ) -> Result<(), Error> { fn var_as_bstring(var: &str, perm: Permission) -> Option { perm.check_opt(var) @@ -418,11 +426,33 @@ fn apply_environment_overrides( .new_section("gitoxide", Some(Cow::Borrowed("objects".into()))) .expect("statically known valid section name"); - for (var, key) in [ - ("GIT_NO_REPLACE_OBJECTS", "noReplace"), - ("GIT_REPLACE_REF_BASE", "replaceRefBase"), + for (var, key, permission) in [ + ("GIT_NO_REPLACE_OBJECTS", "noReplace", git_prefix), + ("GIT_REPLACE_REF_BASE", "replaceRefBase", git_prefix), + ("GITOXIDE_OBJECT_CACHE_MEMORY", "cacheLimit", gitoxide_prefix), ] { - if let Some(value) = var_as_bstring(var, git_prefix) { + if let Some(value) = var_as_bstring(var, permission) { + section.push_with_comment( + key.try_into().expect("statically known to be valid"), + Some(value.as_ref()), + format!("from {var}").as_str(), + ); + } + } + + if section.num_values() == 0 { + let id = section.id(); + env_override.remove_section_by_id(id); + } + } + + { + let mut section = env_override + .new_section("core", None) + .expect("statically known valid section name"); + + for (var, key) in [("GITOXIDE_PACK_CACHE_MEMORY", "deltaBaseCacheLimit")] { + if let Some(value) = var_as_bstring(var, gitoxide_prefix) { section.push_with_comment( key.try_into().expect("statically known to be valid"), Some(value.as_ref()), diff --git a/git-repository/src/config/cache/util.rs b/git-repository/src/config/cache/util.rs index b74938ddbc3..f17cfcdf78f 100644 --- a/git-repository/src/config/cache/util.rs +++ b/git-repository/src/config/cache/util.rs @@ -104,6 +104,31 @@ pub(crate) fn reflog_or_default( }) } +/// Return `(pack_cache_bytes, object_cache_bytes)` as parsed from git-config +pub(crate) fn parse_object_caches( + config: &git_config::File<'static>, + lenient: bool, + mut filter_config_section: fn(&git_config::file::Metadata) -> bool, +) -> Result<(Option, usize), Error> { + let key = "core.deltaBaseCacheLimit"; + let pack_cache_bytes = config + .integer_filter_by_key(key, &mut filter_config_section) + .transpose() + .with_leniency(lenient) + .map_err(|err| Error::Value { source: err, key })?; + let key = "gitoxide.objects.cacheLimit"; + let object_cache_bytes = config + .integer_filter_by_key(key, &mut filter_config_section) + .transpose() + .with_leniency(lenient) + .map_err(|err| Error::Value { source: err, key })? + .unwrap_or_default(); + Ok(( + pack_cache_bytes.and_then(|v| v.try_into().ok()), + object_cache_bytes.try_into().unwrap_or_default(), + )) +} + pub(crate) fn parse_core_abbrev( config: &git_config::File<'static>, object_hash: git_hash::Kind, diff --git a/git-repository/src/config/mod.rs b/git-repository/src/config/mod.rs index 7a9ec4ccf9d..a6c0e451f62 100644 --- a/git-repository/src/config/mod.rs +++ b/git-repository/src/config/mod.rs @@ -195,6 +195,11 @@ pub(crate) struct Cache { pub(crate) url_scheme: OnceCell, /// The algorithm to use when diffing blobs pub(crate) diff_algorithm: OnceCell, + /// The amount of bytes to use for a memory backed delta pack cache. If `Some(0)`, no cache is used, if `None` + /// a standard cache is used which costs near to nothing and always pays for itself. + pub(crate) pack_cache_bytes: Option, + /// The amount of bytes to use for caching whole objects, or 0 to turn it off entirely. + pub(crate) object_cache_bytes: usize, /// The config section filter from the options used to initialize this instance. Keep these in sync! filter_config_section: fn(&git_config::file::Metadata) -> bool, /// The object kind to pick if a prefix is ambiguous. diff --git a/git-repository/src/lib.rs b/git-repository/src/lib.rs index 38eea783eca..4006eb92ad1 100644 --- a/git-repository/src/lib.rs +++ b/git-repository/src/lib.rs @@ -34,9 +34,6 @@ //! Use the `cache-efficiency-debug` cargo feature to learn how efficient the cache actually is - it's easy to end up with lowered //! performance if the cache is not hit in 50% of the time. //! -//! Environment variables can also be used for configuration if the application is calling -//! [`apply_environment()`][crate::Repository::apply_environment()]. -//! //! ### Shortcomings & Limitations //! //! - Only a single `crate::object` or derivatives can be held in memory at a time, _per `Easy*`_. diff --git a/git-repository/src/repository/cache.rs b/git-repository/src/repository/cache.rs index 2dd03767a9c..0a43719aa08 100644 --- a/git-repository/src/repository/cache.rs +++ b/git-repository/src/repository/cache.rs @@ -26,67 +26,4 @@ impl crate::Repository { self.object_cache_size(bytes) } } - - /// Read well-known environment variables related to caches and apply them to this instance, but not to clones of it - each - /// needs their own configuration. - /// - /// Note that environment configuration never fails due to invalid environment values, but it should be used with caution as it - /// could be used to cause high memory consumption. - /// - /// Use the `GITOXIDE_DISABLE_PACK_CACHE` environment variable to turn off any pack cache, which can be beneficial when it's known that - /// the cache efficiency is low. Use `GITOXIDE_PACK_CACHE_MEMORY=512MB` to use up to 512MB of RAM for the pack delta base - /// cache. If none of these are set, the default cache is fast enough to nearly never cause a (marginal) slow-down while providing - /// some gains most of the time. Note that the value given is _per-thread_. - /// - /// Use the `GITOXIDE_OBJECT_CACHE_MEMORY=16mb` to set the given amount of memory to store full objects, on a per-thread basis. - pub fn apply_environment(self) -> Self { - // We have no cache types available without this flag currently. Maybe this should change at some point. - #[cfg(not(feature = "max-performance-safe"))] - return self; - #[cfg(feature = "max-performance-safe")] - { - let pack_cache_disabled = std::env::var_os("GITOXIDE_DISABLE_PACK_CACHE").is_some(); - let mut this = self; - if !pack_cache_disabled { - let bytes = parse_bytes_from_var("GITOXIDE_PACK_CACHE_MEMORY"); - let new_pack_cache = move || -> Box { - match bytes { - Some(bytes) => Box::new(git_pack::cache::lru::MemoryCappedHashmap::new(bytes)), - None => Box::new(git_pack::cache::lru::StaticLinkedList::<64>::default()), - } - }; - this.objects.set_pack_cache(new_pack_cache); - } else { - this.objects.unset_pack_cache(); - } - - if let Some(bytes) = parse_bytes_from_var("GITOXIDE_OBJECT_CACHE_MEMORY") { - this.objects - .set_object_cache(move || Box::new(git_pack::cache::object::MemoryCappedHashmap::new(bytes))); - } - this - } - } -} - -#[cfg(feature = "max-performance-safe")] -fn parse_bytes_from_var(name: &str) -> Option { - std::env::var(name) - .ok() - .and_then(|v| { - byte_unit::Byte::from_str(&v) - .map_err(|err| log::warn!("Failed to parse {:?} into byte unit for pack cache: {}", v, err)) - .ok() - }) - .and_then(|unit| { - unit.get_bytes() - .try_into() - .map_err(|err| { - log::warn!( - "Parsed bytes value is not representable as usize. Defaulting to standard pack cache: {}", - err - ) - }) - .ok() - }) } diff --git a/git-repository/src/repository/init.rs b/git-repository/src/repository/init.rs index 781dd640bbb..aa52496103b 100644 --- a/git-repository/src/repository/init.rs +++ b/git-repository/src/repository/init.rs @@ -10,20 +10,12 @@ impl crate::Repository { linked_worktree_options: crate::open::Options, index: crate::worktree::IndexStorage, ) -> Self { + let objects = setup_objects(objects, &config); crate::Repository { bufs: RefCell::new(Vec::with_capacity(4)), work_tree, common_dir, - objects: { - #[cfg(feature = "max-performance-safe")] - { - objects.with_pack_cache(|| Box::new(git_pack::cache::lru::StaticLinkedList::<64>::default())) - } - #[cfg(not(feature = "max-performance-safe"))] - { - objects - } - }, + objects, refs, config, options: linked_worktree_options, @@ -36,3 +28,28 @@ impl crate::Repository { self.into() } } + +#[cfg_attr(not(feature = "max-performance-safe"), allow(unused_variables, unused_mut))] +fn setup_objects(mut objects: crate::OdbHandle, config: &crate::config::Cache) -> crate::OdbHandle { + #[cfg(feature = "max-performance-safe")] + { + match config.pack_cache_bytes { + None => objects.set_pack_cache(|| Box::new(git_pack::cache::lru::StaticLinkedList::<64>::default())), + Some(0) => objects.unset_pack_cache(), + Some(bytes) => objects.set_pack_cache(move || -> Box { + Box::new(git_pack::cache::lru::MemoryCappedHashmap::new(bytes)) + }), + }; + if config.object_cache_bytes == 0 { + objects.unset_object_cache(); + } else { + let bytes = config.object_cache_bytes; + objects.set_object_cache(move || Box::new(git_pack::cache::object::MemoryCappedHashmap::new(bytes))); + } + objects + } + #[cfg(not(feature = "max-performance-safe"))] + { + objects + } +} diff --git a/git-repository/src/repository/permissions.rs b/git-repository/src/repository/permissions.rs index a44a06ba3be..45dfa046e6d 100644 --- a/git-repository/src/repository/permissions.rs +++ b/git-repository/src/repository/permissions.rs @@ -82,6 +82,8 @@ pub struct Environment { /// Note that identity related environment variables prefixed with `GIT_` are falling under the /// `git_prefix` permission, like `GIT_AUTHOR_NAME`. pub identity: git_sec::Permission, + /// Decide if `gitoxide` specific variables may be read, prefixed with `GITOXIDE_`. + pub gitoxide_prefix: git_sec::Permission, } impl Environment { @@ -94,6 +96,7 @@ impl Environment { ssh_prefix: git_sec::Permission::Allow, http_transport: git_sec::Permission::Allow, identity: git_sec::Permission::Allow, + gitoxide_prefix: git_sec::Permission::Allow, } } } @@ -140,6 +143,7 @@ impl Permissions { git_prefix: deny, http_transport: deny, identity: deny, + gitoxide_prefix: deny, } }, } diff --git a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz index fde74bd6e42..d45b7e93835 100644 --- a/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz +++ b/git-repository/tests/fixtures/generated-archives/make_config_repos.tar.xz @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:607f9cd2af225da2cff5640855d557b363949bd667454e772ae49a63deb21831 -size 13284 +oid sha256:3d893d90b1870819d92b25f84214d10e7565f6cab71032959659f284642dc5a4 +size 13808 diff --git a/git-repository/tests/fixtures/make_config_repos.sh b/git-repository/tests/fixtures/make_config_repos.sh index f4659a5d793..783b9c3fa2c 100644 --- a/git-repository/tests/fixtures/make_config_repos.sh +++ b/git-repository/tests/fixtures/make_config_repos.sh @@ -99,3 +99,15 @@ git init http-proxy-authenticated followRedirects EOF ) + +git init object-caches +(cd object-caches + git config core.deltaBaseCacheLimit 128m + git config gitoxide.objects.cacheLimit 16m +) + +git init disabled-object-caches +(cd disabled-object-caches + git config core.deltaBaseCacheLimit 0 + git config gitoxide.objects.cacheLimit 0 +) diff --git a/git-repository/tests/repository/open.rs b/git-repository/tests/repository/open.rs index 49689b4d433..6bc29aaa33b 100644 --- a/git-repository/tests/repository/open.rs +++ b/git-repository/tests/repository/open.rs @@ -45,6 +45,29 @@ mod submodules { } } +mod object_caches { + use crate::util::named_subrepo_opts; + use git_repository as git; + + #[test] + fn default_git_and_custom_caches() -> crate::Result { + let opts = git::open::Options::isolated(); + let repo = named_subrepo_opts("make_config_repos.sh", "object-caches", opts)?; + assert!(repo.objects.has_object_cache()); + assert!(repo.objects.has_pack_cache()); + Ok(()) + } + + #[test] + fn disabled() -> crate::Result { + let opts = git::open::Options::isolated(); + let repo = named_subrepo_opts("make_config_repos.sh", "disabled-object-caches", opts)?; + assert!(!repo.objects.has_object_cache()); + assert!(!repo.objects.has_pack_cache()); + Ok(()) + } +} + mod with_overrides { use crate::util::named_subrepo_opts; use git_object::bstr::BStr; @@ -80,7 +103,9 @@ mod with_overrides { .set("GIT_AUTHOR_NAME", "author name") .set("GIT_AUTHOR_EMAIL", "author email") .set("GIT_AUTHOR_DATE", default_date) - .set("EMAIL", "user email"); + .set("EMAIL", "user email") + .set("GITOXIDE_PACK_CACHE_MEMORY", "0") + .set("GITOXIDE_OBJECT_CACHE_MEMORY", "5m"); let mut opts = git::open::Options::isolated() .config_overrides([ "http.userAgent=agent-from-api", @@ -95,6 +120,7 @@ mod with_overrides { opts.permissions.env.git_prefix = Permission::Allow; opts.permissions.env.http_transport = Permission::Allow; opts.permissions.env.identity = Permission::Allow; + opts.permissions.env.gitoxide_prefix = Permission::Allow; let repo = named_subrepo_opts("make_config_repos.sh", "http-config", opts)?; let config = repo.config_snapshot(); assert_eq!( @@ -175,6 +201,8 @@ mod with_overrides { ("gitoxide.commit.authorDate", default_date), ("gitoxide.commit.committerDate", default_date), ("gitoxide.user.emailFallback", "user email"), + ("core.deltaBaseCacheLimit", "0"), + ("gitoxide.objects.cacheLimit", "5m"), ] { assert_eq!( config diff --git a/src/plumbing/progress.rs b/src/plumbing/progress.rs index c78086de356..7d94461f482 100644 --- a/src/plumbing/progress.rs +++ b/src/plumbing/progress.rs @@ -125,7 +125,10 @@ static GIT_CONFIG: &[Record] = &[ }, Record { config: "core.deltaBaseCacheLimit", - usage: NotApplicable { reason: "we use a small 64 slot pack delta cache by default, which can be replaced with larger caches as determined by the algorithm. This keeps memory usage low and is fast enough" } + usage: InModule { + name: "repository::cache", + deviation: Some("if unset, we default to a small 64 slot fixed-size cache that holds at most 64 full delta base objects of any size. Overridable by 'GITOXIDE_PACK_CACHE_MEMORY'. Set to 0 to deactivate it entirely.") + } }, Record { config: "core.bigFileThreshold", @@ -845,6 +848,13 @@ static GIT_CONFIG: &[Record] = &[ deviation: Some("corresponds to the EMAIL environment variable and is a fallback for `user.email`") } }, + Record { + config: "gitoxide.objects.cacheLimit", + usage: InModule { + name: "repository::cache", + deviation: Some("corresponds to the GITOXIDE_OBJECT_CACHE_MEMORY environment variable. If unset or 0, there is no object cache") + } + }, ]; /// A programmatic way to record and display progress. From c4f68bf775b854625d901fe0bfcbdd38f656d408 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Tue, 29 Nov 2022 20:46:27 +0100 Subject: [PATCH 29/29] adapt to changes in `git-repository` --- cargo-smart-release/src/context.rs | 2 +- gitoxide-core/src/hours/mod.rs | 2 +- gitoxide-core/src/index/checkout.rs | 4 +--- src/plumbing/main.rs | 3 +-- 4 files changed, 4 insertions(+), 7 deletions(-) diff --git a/cargo-smart-release/src/context.rs b/cargo-smart-release/src/context.rs index ec5569e64ee..8902fc3e32b 100644 --- a/cargo-smart-release/src/context.rs +++ b/cargo-smart-release/src/context.rs @@ -26,7 +26,7 @@ impl Context { ) -> anyhow::Result { let meta = cargo_metadata::MetadataCommand::new().exec()?; let root = meta.workspace_root.clone(); - let repo = git::discover(&root)?.apply_environment(); + let repo = git::discover(&root)?; let crates_index = crate::crates_index::Index::new_cargo_default()?; let history = (force_history_segmentation || matches!(bump, BumpSpec::Auto) diff --git a/gitoxide-core/src/hours/mod.rs b/gitoxide-core/src/hours/mod.rs index 36e54acba87..a76fff56f5f 100644 --- a/gitoxide-core/src/hours/mod.rs +++ b/gitoxide-core/src/hours/mod.rs @@ -53,7 +53,7 @@ where W: io::Write, P: Progress, { - let repo = git::discover(working_dir)?.apply_environment(); + let repo = git::discover(working_dir)?; let commit_id = repo.rev_parse_single(rev_spec)?.detach(); let mut string_heap = BTreeSet::<&'static [u8]>::new(); let needs_stats = file_stats || line_stats; diff --git a/gitoxide-core/src/index/checkout.rs b/gitoxide-core/src/index/checkout.rs index 4f783710b51..ff06c479fd0 100644 --- a/gitoxide-core/src/index/checkout.rs +++ b/gitoxide-core/src/index/checkout.rs @@ -26,9 +26,7 @@ pub fn checkout_exclusive( thread_limit, }: index::checkout_exclusive::Options, ) -> anyhow::Result<()> { - let repo = repo - .map(|dir| git_repository::discover(dir).map(|r| r.apply_environment())) - .transpose()?; + let repo = repo.map(git_repository::discover).transpose()?; let dest_directory = dest_directory.as_ref(); if dest_directory.exists() { diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 49b21e84e48..8cdeeb5dedb 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -89,8 +89,7 @@ pub fn main() -> Result<()> { mapping.full.modify(to_match_settings); mapping.reduced.modify(to_match_settings); let mut repo = git::ThreadSafeRepository::discover_opts(repository, Default::default(), mapping) - .map(git::Repository::from) - .map(|r| r.apply_environment())?; + .map(git::Repository::from)?; if !config.is_empty() { repo.config_snapshot_mut() .append_config(config.iter(), git::config::Source::Cli)