Merge branch 'core-abbrev-handling'

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<bool>) -> Result<Self, Error> {
+    pub fn new(
+        git_dir: &std::path::Path,
+        git_dir_trust: git_sec::Trust,
+        lossy: Option<bool>,
+        lenient: bool,
+    ) -> Result<Self, Error> {
         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::<Integer>("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<Self, Error> {
         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<bool>) -> git_config::file::init::Options<'static>
     }
 }
 
-fn config_bool(config: &git_config::File<'_>, key: &str, default: bool) -> Result<bool, Error> {
+fn config_bool(config: &git_config::File<'_>, key: &str, default: bool, lenient: bool) -> Result<bool, Error> {
     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<git_ref::store::WriteReflog> {
@@ -315,3 +305,40 @@ fn query_refupdates(config: &git_config::File<'static>) -> Option<git_ref::store
             .unwrap_or(git_ref::store::WriteReflog::Disable)
     })
 }
+
+fn parse_core_abbrev(config: &git_config::File<'static>, object_hash: git_hash::Kind) -> Result<Option<usize>, 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),
+    }
+}

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)]
@@ -60,17 +60,33 @@ 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,
     pub(crate) permissions: Permissions,
     pub(crate) git_dir_trust: Option<git_sec::Trust>,
     pub(crate) filter_config_section: Option<fn(&git_config::file::Metadata) -> bool>,
     pub(crate) lossy_config: Option<bool>,
+    pub(crate) lenient_config: bool,
     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 {
@@ -103,23 +119,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 +190,22 @@ 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 cause an error even if these can safely be defaulted.
+    ///
+    /// 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
+    }
+
     /// Open a repository at `path` with the options set so far.
     pub fn open(self, path: impl Into<PathBuf>) -> Result<ThreadSafeRepository, Error> {
         ThreadSafeRepository::open_opts(path, self)
@@ -213,6 +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: true,
             },
             git_sec::Trust::Reduced => Options {
                 object_store_slots: git_odb::store::init::Slots::Given(32), // limit resource usage
@@ -221,6 +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: true,
                 lossy_config: None,
             },
         }
@@ -231,7 +243,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 +326,7 @@ impl ThreadSafeRepository {
             filter_config_section,
             ref replacement_objects,
             lossy_config,
+            lenient_config,
             bail_if_untrusted,
             permissions: Permissions { ref env, config },
         } = options;
@@ -328,7 +341,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 +364,7 @@ impl ThreadSafeRepository {
             home.as_deref(),
             env.clone(),
             config,
+            lenient_config,
         )?;
 
         if bail_if_untrusted && git_dir_trust != git_sec::Trust::Full {

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 {

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,24 @@ 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_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(worktree_dir.path()).is_ok(),
+        "By default gitoxide acts like `libgit2` here and we prefer to be lenient when possible"
+    );
     Ok(())
 }