From 067c3342f3564dd7f152a720e93e3aa590ae6524 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 17 Aug 2022 10:16:17 +0800 Subject: [PATCH 1/3] =?UTF-8?q?feat:=20`open::Options::lenient=5Fconfig(?= =?UTF-8?q?=E2=80=A6)`=20to=20default=20otherwise=20invalid=20configuratio?= =?UTF-8?q?n=20values=20where=20possible?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Originally required by https://github.com/starship/starship/issues/4266 . --- git-repository/src/config/cache.rs | 105 ++++++++++++------- git-repository/src/open.rs | 42 ++++---- git-repository/src/repository/permissions.rs | 23 +++- git-repository/tests/id/mod.rs | 18 ++++ 4 files changed, 127 insertions(+), 61 deletions(-) diff --git a/git-repository/src/config/cache.rs b/git-repository/src/config/cache.rs index 53a69ae7ee1..8943985f4cd 100644 --- a/git-repository/src/config/cache.rs +++ b/git-repository/src/config/cache.rs @@ -20,7 +20,12 @@ pub(crate) struct StageOne { /// Initialization impl StageOne { - pub fn new(git_dir: &std::path::Path, git_dir_trust: git_sec::Trust, lossy: Option) -> Result { + pub fn new( + git_dir: &std::path::Path, + git_dir_trust: git_sec::Trust, + lossy: Option, + lenient: bool, + ) -> Result { let mut buf = Vec::with_capacity(512); let config = { let config_path = git_dir.join("config"); @@ -38,7 +43,7 @@ impl StageOne { )? }; - let is_bare = config_bool(&config, "core.bare", false)?; + let is_bare = config_bool(&config, "core.bare", false, lenient)?; let repo_format_version = config .value::("core", None, "repositoryFormatVersion") .map_or(0, |v| v.to_decimal().unwrap_or_default()); @@ -99,6 +104,7 @@ impl Cache { env: use_env, includes: use_includes, }: repository::permissions::Config, + lenient_config: bool, ) -> Result { let options = git_config::file::init::Options { includes: if use_includes { @@ -174,45 +180,25 @@ impl Cache { globals }; - let excludes_file = config + let excludes_file = match config .path_filter("core", None, "excludesFile", &mut filter_config_section) .map(|p| p.interpolate(options.includes.interpolate).map(|p| p.into_owned())) - .transpose()?; + .transpose() + { + Ok(f) => f, + Err(_err) if lenient_config => None, + Err(err) => return Err(err.into()), + }; - let mut hex_len = None; - if let Some(hex_len_str) = config.string("core", None, "abbrev") { - if hex_len_str.trim().is_empty() { - return Err(Error::EmptyValue { key: "core.abbrev" }); - } - if !hex_len_str.eq_ignore_ascii_case(b"auto") { - let value_bytes = hex_len_str.as_ref(); - if let Ok(false) = Boolean::try_from(value_bytes).map(Into::into) { - hex_len = object_hash.len_in_hex().into(); - } else { - let value = Integer::try_from(value_bytes) - .map_err(|_| Error::CoreAbbrev { - value: hex_len_str.clone().into_owned(), - max: object_hash.len_in_hex() as u8, - })? - .to_decimal() - .ok_or_else(|| Error::CoreAbbrev { - value: hex_len_str.clone().into_owned(), - max: object_hash.len_in_hex() as u8, - })?; - if value < 4 || value as usize > object_hash.len_in_hex() { - return Err(Error::CoreAbbrev { - value: hex_len_str.clone().into_owned(), - max: object_hash.len_in_hex() as u8, - }); - } - hex_len = Some(value as usize); - } - } - } + let hex_len = match parse_core_abbrev(&config, object_hash) { + Ok(v) => v, + Err(_err) if lenient_config => None, + Err(err) => return Err(err), + }; let reflog = query_refupdates(&config); - let ignore_case = config_bool(&config, "core.ignoreCase", false)?; - let use_multi_pack_index = config_bool(&config, "core.multiPackIndex", true)?; + 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 = config.string("core", None, "disambiguate").and_then(|value| { Some(match value.as_ref().as_ref() { b"commit" => ObjectKindHint::Commit, @@ -292,15 +278,19 @@ fn base_options(lossy: Option) -> git_config::file::init::Options<'static> } } -fn config_bool(config: &git_config::File<'_>, key: &str, default: bool) -> Result { +fn config_bool(config: &git_config::File<'_>, key: &str, default: bool, lenient: bool) -> Result { let (section, key) = key.split_once('.').expect("valid section.key format"); - config + match config .boolean(section, None, key) .unwrap_or(Ok(default)) .map_err(|err| Error::DecodeBoolean { value: err.input, key: key.into(), - }) + }) { + Ok(v) => Ok(v), + Err(_err) if lenient => Ok(default), + Err(err) => Err(err), + } } fn query_refupdates(config: &git_config::File<'static>) -> Option { @@ -315,3 +305,40 @@ fn query_refupdates(config: &git_config::File<'static>) -> Option, object_hash: git_hash::Kind) -> Result, Error> { + match config.string("core", None, "abbrev") { + Some(hex_len_str) => { + if hex_len_str.trim().is_empty() { + return Err(Error::EmptyValue { key: "core.abbrev" }); + } + if hex_len_str.trim().eq_ignore_ascii_case(b"auto") { + Ok(None) + } else { + let value_bytes = hex_len_str.as_ref(); + if let Ok(false) = Boolean::try_from(value_bytes).map(Into::into) { + Ok(object_hash.len_in_hex().into()) + } else { + let value = Integer::try_from(value_bytes) + .map_err(|_| Error::CoreAbbrev { + value: hex_len_str.clone().into_owned(), + max: object_hash.len_in_hex() as u8, + })? + .to_decimal() + .ok_or_else(|| Error::CoreAbbrev { + value: hex_len_str.clone().into_owned(), + max: object_hash.len_in_hex() as u8, + })?; + if value < 4 || value as usize > object_hash.len_in_hex() { + return Err(Error::CoreAbbrev { + value: hex_len_str.clone().into_owned(), + max: object_hash.len_in_hex() as u8, + }); + } + Ok(Some(value as usize)) + } + } + } + None => Ok(None), + } +} diff --git a/git-repository/src/open.rs b/git-repository/src/open.rs index f78681911bf..b7dc61abfe6 100644 --- a/git-repository/src/open.rs +++ b/git-repository/src/open.rs @@ -2,7 +2,7 @@ use std::path::PathBuf; use git_features::threading::OwnShared; -use crate::{config, config::cache::interpolate_context, permission, permissions, Permissions, ThreadSafeRepository}; +use crate::{config, config::cache::interpolate_context, permission, Permissions, ThreadSafeRepository}; /// A way to configure the usage of replacement objects, see `git replace`. #[derive(Debug, Clone)] @@ -68,6 +68,7 @@ pub struct Options { pub(crate) git_dir_trust: Option, pub(crate) filter_config_section: Option bool>, pub(crate) lossy_config: Option, + pub(crate) lenient_config: bool, pub(crate) bail_if_untrusted: bool, } @@ -103,23 +104,7 @@ 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 { - config: permissions::Config { - system: false, - git: false, - user: false, - env: false, - includes: false, - }, - env: { - let deny = permission::env_var::Resource::resource(git_sec::Permission::Deny); - permissions::Environment { - xdg_config_home: deny.clone(), - home: deny.clone(), - git_prefix: deny, - } - }, - }) + Options::default().permissions(Permissions::isolated()) } } @@ -190,12 +175,23 @@ impl Options { /// 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. + /// 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 be defaulted to acceptable values where when possible, + /// instead of yielding an error during startup. + /// + /// This is recommended for all applications that prefer usability over correctness. `git` itslef by default is not lenient + /// towards malconfigured repositories. + pub fn lenient_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) @@ -213,6 +209,7 @@ impl git_sec::trust::DefaultForLevel for Options { filter_config_section: Some(config::section::is_trusted), lossy_config: None, bail_if_untrusted: false, + lenient_config: false, }, git_sec::Trust::Reduced => Options { object_store_slots: git_odb::store::init::Slots::Given(32), // limit resource usage @@ -221,6 +218,7 @@ impl git_sec::trust::DefaultForLevel for Options { git_dir_trust: git_sec::Trust::Reduced.into(), filter_config_section: Some(config::section::is_trusted), bail_if_untrusted: false, + lenient_config: false, lossy_config: None, }, } @@ -231,7 +229,7 @@ impl git_sec::trust::DefaultForLevel for Options { #[derive(Debug, thiserror::Error)] #[allow(missing_docs)] pub enum Error { - #[error(transparent)] + #[error("Failed to load the git configuration")] Config(#[from] config::Error), #[error(transparent)] NotARepository(#[from] git_discover::is_git::Error), @@ -314,6 +312,7 @@ impl ThreadSafeRepository { filter_config_section, ref replacement_objects, lossy_config, + lenient_config, bail_if_untrusted, permissions: Permissions { ref env, config }, } = options; @@ -328,7 +327,7 @@ impl ThreadSafeRepository { .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)?; + 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; @@ -351,6 +350,7 @@ impl ThreadSafeRepository { home.as_deref(), env.clone(), config, + lenient_config, )?; if bail_if_untrusted && git_dir_trust != git_sec::Trust::Full { diff --git a/git-repository/src/repository/permissions.rs b/git-repository/src/repository/permissions.rs index b996451f783..b97937f4ade 100644 --- a/git-repository/src/repository/permissions.rs +++ b/git-repository/src/repository/permissions.rs @@ -26,8 +26,8 @@ pub struct Config { /// Whether to use the user configuration. /// This is usually `~/.gitconfig` on unix. pub user: bool, - /// Whether to use worktree configuration from `config.worktree`. // TODO: figure out how this really applies and provide more information here. + // Whether to use worktree configuration from `config.worktree`. // pub worktree: bool, /// Whether to use the configuration from environment variables. pub env: bool, @@ -100,6 +100,27 @@ impl Permissions { config: Config::all(), } } + + /// Don't read any but the local git configuration and deny reading any environment variables. + pub fn isolated() -> Self { + Permissions { + config: Config { + system: false, + git: false, + user: false, + env: false, + includes: false, + }, + env: { + let deny = permission::env_var::Resource::resource(git_sec::Permission::Deny); + Environment { + xdg_config_home: deny.clone(), + home: deny.clone(), + git_prefix: deny, + } + }, + } + } } impl git_sec::trust::DefaultForLevel for Permissions { diff --git a/git-repository/tests/id/mod.rs b/git-repository/tests/id/mod.rs index e8837b3bcd1..e128ca2f10f 100644 --- a/git-repository/tests/id/mod.rs +++ b/git-repository/tests/id/mod.rs @@ -1,5 +1,6 @@ use std::cmp::Ordering; +use git_repository as git; use git_repository::prelude::ObjectIdExt; use git_testtools::hex_to_id; @@ -22,6 +23,23 @@ fn prefix() -> crate::Result { let prefix = id.shorten()?; assert_eq!(prefix.cmp_oid(&id), Ordering::Equal); assert_eq!(prefix.hex_len(), 5, "preconfigured via core.abbrev in the repo file"); + + assert!( + git_testtools::run_git(worktree_dir.path(), &["config", "core.abbrev", ""])?.success(), + "set core abbrev value to empty successfully" + ); + + assert!( + matches!( + git_repository::open(worktree_dir.path()).unwrap_err(), + git::open::Error::Config(git::config::Error::EmptyValue { .. }) + ), + "an empty core.abbrev fails the open operation in strict config mode, emulating git behaviour" + ); + assert!( + git_repository::open_opts(worktree_dir.path(), git::open::Options::isolated().lenient_config(true)).is_ok(), + "but it can be made to work when we are lenient (good for APIs)" + ); Ok(()) } From 0235111a4fcc40c7b57d973bfce27a66eddea901 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 17 Aug 2022 10:28:11 +0800 Subject: [PATCH 2/3] change: Invert behaviour to `open::Options::strict_config()`, with lenient being the default. This means API users will get libgit2 behaviour but commands like `gix` can change options to emulate `git` behaviour. --- git-repository/src/open.rs | 32 +++++++++++++++++++++++--------- git-repository/tests/id/mod.rs | 7 ++++--- src/plumbing/main.rs | 5 ++++- 3 files changed, 31 insertions(+), 13 deletions(-) diff --git a/git-repository/src/open.rs b/git-repository/src/open.rs index b7dc61abfe6..21658f236ea 100644 --- a/git-repository/src/open.rs +++ b/git-repository/src/open.rs @@ -60,7 +60,7 @@ impl ReplacementObjects { } /// The options used in [`ThreadSafeRepository::open_opts`] -#[derive(Default, Clone)] +#[derive(Clone)] pub struct Options { pub(crate) object_store_slots: git_odb::store::init::Slots, pub(crate) replacement_objects: ReplacementObjects, @@ -72,6 +72,21 @@ pub struct Options { pub(crate) bail_if_untrusted: bool, } +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, + } + } +} + #[derive(Default, Clone)] #[allow(dead_code)] pub(crate) struct EnvironmentOverrides { @@ -182,13 +197,12 @@ impl Options { self } - /// If set, default is false, invalid configuration values will be defaulted to acceptable values where when possible, - /// instead of yielding an error during startup. + /// 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 usability over correctness. `git` itslef by default is not lenient - /// towards malconfigured repositories. - pub fn lenient_config(mut self, toggle: bool) -> Self { - self.lenient_config = toggle; + /// This is recommended for all applications that prefer correctness over usability. + /// `git` itself by defaults to strict configuration mode to let you know if configuration is incorrect. + pub fn strict_config(mut self, toggle: bool) -> Self { + self.lenient_config = !toggle; self } @@ -209,7 +223,7 @@ impl git_sec::trust::DefaultForLevel for Options { filter_config_section: Some(config::section::is_trusted), lossy_config: None, bail_if_untrusted: false, - lenient_config: false, + lenient_config: true, }, git_sec::Trust::Reduced => Options { object_store_slots: git_odb::store::init::Slots::Given(32), // limit resource usage @@ -218,7 +232,7 @@ impl git_sec::trust::DefaultForLevel for Options { git_dir_trust: git_sec::Trust::Reduced.into(), filter_config_section: Some(config::section::is_trusted), bail_if_untrusted: false, - lenient_config: false, + lenient_config: true, lossy_config: None, }, } diff --git a/git-repository/tests/id/mod.rs b/git-repository/tests/id/mod.rs index e128ca2f10f..eb555b8f1ee 100644 --- a/git-repository/tests/id/mod.rs +++ b/git-repository/tests/id/mod.rs @@ -31,14 +31,15 @@ fn prefix() -> crate::Result { assert!( matches!( - git_repository::open(worktree_dir.path()).unwrap_err(), + git_repository::open_opts(worktree_dir.path(), git::open::Options::isolated().strict_config(true)) + .unwrap_err(), git::open::Error::Config(git::config::Error::EmptyValue { .. }) ), "an empty core.abbrev fails the open operation in strict config mode, emulating git behaviour" ); assert!( - git_repository::open_opts(worktree_dir.path(), git::open::Options::isolated().lenient_config(true)).is_ok(), - "but it can be made to work when we are lenient (good for APIs)" + git_repository::open(worktree_dir.path()).is_ok(), + "By default gitoxide acts like `libgit2` here and we prefer to be lenient when possible" ); Ok(()) } diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index 7385b9d8f6f..f8970414513 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -54,7 +54,10 @@ pub fn main() -> Result<()> { use git_repository as git; let repository = args.repository; let repository = move || { - git::ThreadSafeRepository::discover(repository) + let mut mapping: git::sec::trust::Mapping = Default::default(); + mapping.full = mapping.full.strict_config(true); + mapping.reduced = mapping.reduced.strict_config(true); + git::ThreadSafeRepository::discover_opts(repository, Default::default(), mapping) .map(git::Repository::from) .map(|r| r.apply_environment()) }; From 6a9c58fde7ca4a52fa1c3225974a2019e7d93168 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Wed, 17 Aug 2022 10:37:48 +0800 Subject: [PATCH 3/3] Control which command is lenient or not. That way `gix-config` can be lenient. --- src/plumbing/main.rs | 51 ++++++++++++++++++++++++++++++++------------ 1 file changed, 37 insertions(+), 14 deletions(-) diff --git a/src/plumbing/main.rs b/src/plumbing/main.rs index f8970414513..a8706a1792c 100644 --- a/src/plumbing/main.rs +++ b/src/plumbing/main.rs @@ -53,10 +53,15 @@ pub fn main() -> Result<()> { let object_hash = args.object_hash; use git_repository as git; let repository = args.repository; - let repository = move || { + enum Mode { + Strict, + Lenient, + } + let repository = move |mode: Mode| { let mut mapping: git::sec::trust::Mapping = Default::default(); - mapping.full = mapping.full.strict_config(true); - mapping.reduced = mapping.reduced.strict_config(true); + let toggle = matches!(mode, Mode::Strict); + mapping.full = mapping.full.strict_config(toggle); + mapping.reduced = mapping.reduced.strict_config(toggle); git::ThreadSafeRepository::discover_opts(repository, Default::default(), mapping) .map(git::Repository::from) .map(|r| r.apply_environment()) @@ -88,7 +93,7 @@ pub fn main() -> Result<()> { progress, progress_keep_open, None, - move |_progress, out, _err| core::repository::config::list(repository()?, filter, format, out), + move |_progress, out, _err| core::repository::config::list(repository(Mode::Lenient)?, filter, format, out), ) .map(|_| ()), Subcommands::Free(subcommands) => match subcommands { @@ -505,7 +510,7 @@ pub fn main() -> Result<()> { core::repository::verify::PROGRESS_RANGE, move |progress, out, _err| { core::repository::verify::integrity( - repository()?, + repository(Mode::Strict)?, out, progress, &should_interrupt, @@ -525,7 +530,9 @@ pub fn main() -> Result<()> { progress, progress_keep_open, None, - move |_progress, out, _err| core::repository::revision::previous_branches(repository()?, out, format), + move |_progress, out, _err| { + core::repository::revision::previous_branches(repository(Mode::Lenient)?, out, format) + }, ), revision::Subcommands::Explain { spec } => prepare_and_run( "revision-explain", @@ -547,7 +554,7 @@ pub fn main() -> Result<()> { None, move |_progress, out, _err| { core::repository::revision::resolve( - repository()?, + repository(Mode::Strict)?, specs, out, core::repository::revision::resolve::Options { @@ -577,7 +584,7 @@ pub fn main() -> Result<()> { None, move |_progress, out, err| { core::repository::commit::describe( - repository()?, + repository(Mode::Strict)?, rev_spec.as_deref(), out, err, @@ -606,7 +613,14 @@ pub fn main() -> Result<()> { progress_keep_open, None, move |_progress, out, _err| { - core::repository::tree::entries(repository()?, treeish.as_deref(), recursive, extended, format, out) + core::repository::tree::entries( + repository(Mode::Strict)?, + treeish.as_deref(), + recursive, + extended, + format, + out, + ) }, ), tree::Subcommands::Info { treeish, extended } => prepare_and_run( @@ -616,7 +630,14 @@ pub fn main() -> Result<()> { progress_keep_open, None, move |_progress, out, err| { - core::repository::tree::info(repository()?, treeish.as_deref(), extended, format, out, err) + core::repository::tree::info( + repository(Mode::Strict)?, + treeish.as_deref(), + extended, + format, + out, + err, + ) }, ), }, @@ -627,7 +648,7 @@ pub fn main() -> Result<()> { progress, progress_keep_open, None, - move |_progress, out, _err| core::repository::odb::entries(repository()?, format, out), + move |_progress, out, _err| core::repository::odb::entries(repository(Mode::Strict)?, format, out), ), odb::Subcommands::Info => prepare_and_run( "odb-info", @@ -635,7 +656,7 @@ pub fn main() -> Result<()> { progress, progress_keep_open, None, - move |_progress, out, err| core::repository::odb::info(repository()?, format, out, err), + move |_progress, out, err| core::repository::odb::info(repository(Mode::Strict)?, format, out, err), ), }, Subcommands::Mailmap(cmd) => match cmd { @@ -645,7 +666,9 @@ pub fn main() -> Result<()> { progress, progress_keep_open, None, - move |_progress, out, err| core::repository::mailmap::entries(repository()?, format, out, err), + move |_progress, out, err| { + core::repository::mailmap::entries(repository(Mode::Lenient)?, format, out, err) + }, ), }, Subcommands::Exclude(cmd) => match cmd { @@ -662,7 +685,7 @@ pub fn main() -> Result<()> { move |_progress, out, _err| { use git::bstr::ByteSlice; core::repository::exclude::query( - repository()?, + repository(Mode::Strict)?, if pathspecs.is_empty() { Box::new( stdin_or_bail()?