feat!: make API less error prone by enforcing overrides at instantiation time.

It's made so that overrides can still be applied at a later point.

Author: Byron

gix-submodule/src/access.rs

@@ -2,6 +2,7 @@ use crate::config::{Branch, FetchRecurse, Ignore, Update};
 use crate::{config, File};
 use bstr::BStr;
 use std::borrow::Cow;
+use std::collections::HashSet;
 use std::path::Path;
 
 /// High-Level Access
@@ -26,11 +27,17 @@ impl File {
     ///
     /// Note that these exact names have to be used for querying submodule values.
     pub fn names(&self) -> impl Iterator<Item = &BStr> {
+        let mut seen = HashSet::<&BStr>::default();
         self.config
             .sections_by_name("submodule")
             .into_iter()
             .flatten()
-            .filter_map(|s| s.header().subsection_name())
+            .filter_map(move |s| {
+                s.header()
+                    .subsection_name()
+                    .filter(|_| s.meta().source == crate::init::META_MARKER)
+                    .filter(|name| seen.insert(*name))
+            })
     }
 
     /// Return an iterator of names along with a boolean that indicates the submodule is active (`true`) or inactive  (`false`).

gix-submodule/src/lib.rs

@@ -90,28 +90,41 @@ mod init {
         }
     }
 
+    /// A marker we use when listing names to not pick them up from overridden sections.
+    pub(crate) const META_MARKER: gix_config::Source = gix_config::Source::Api;
+
     /// Lifecycle
     impl File {
         /// Parse `bytes` as git configuration, typically from `.gitmodules`, without doing any further validation.
         /// `path` can be provided to keep track of where the file was read from in the underlying [`config`](Self::config())
         /// instance.
+        /// `config` is used to [apply value overrides](File::append_submodule_overrides), which can be empty if overrides
+        /// should be applied at a later time.
         ///
         /// Future access to the module information is lazy and configuration errors are exposed there on a per-value basis.
         ///
         /// ### Security Considerations
         ///
         /// The information itself should be used with care as it can direct the caller to fetch from remotes. It is, however,
         /// on the caller to assure the input data can be trusted.
-        pub fn from_bytes(bytes: &[u8], path: impl Into<Option<PathBuf>>) -> Result<Self, gix_config::parse::Error> {
-            let metadata = path.into().map_or_else(gix_config::file::Metadata::api, |path| {
-                gix_config::file::Metadata::from(gix_config::Source::Worktree).at(path)
-            });
-            let config = gix_config::File::from_parse_events_no_includes(
+        pub fn from_bytes(
+            bytes: &[u8],
+            path: impl Into<Option<PathBuf>>,
+            config: &gix_config::File<'_>,
+        ) -> Result<Self, gix_config::parse::Error> {
+            let metadata = {
+                let mut meta = gix_config::file::Metadata::from(META_MARKER);
+                meta.path = path.into();
+                meta
+            };
+            let modules = gix_config::File::from_parse_events_no_includes(
                 gix_config::parse::Events::from_bytes_owned(bytes, None)?,
                 metadata,
             );
 
-            Ok(Self { config })
+            let mut res = Self { config: modules };
+            res.append_submodule_overrides(config);
+            Ok(res)
         }
 
         /// Turn ourselves into the underlying parsed configuration file.

gix-submodule/tests/file/baseline.rs

@@ -6,7 +6,9 @@ use std::path::PathBuf;
 #[test]
 fn common_values_and_names_by_path() -> crate::Result {
     let modules = module_files()
-        .map(|(path, stripped)| gix_submodule::File::from_bytes(&std::fs::read(path).unwrap(), stripped))
+        .map(|(path, stripped)| {
+            gix_submodule::File::from_bytes(&std::fs::read(path).unwrap(), stripped, &Default::default())
+        })
         .collect::<Result<Vec<_>, _>>()?;
 
     assert_eq!(

gix-submodule/tests/file/mod.rs

@@ -1,5 +1,5 @@
 fn submodule(bytes: &str) -> gix_submodule::File {
-    gix_submodule::File::from_bytes(bytes.as_bytes(), None).expect("valid module")
+    gix_submodule::File::from_bytes(bytes.as_bytes(), None, &Default::default()).expect("valid module")
 }
 
 mod names_and_active_state {
@@ -13,6 +13,7 @@ mod names_and_active_state {
         Ok(gix_submodule::File::from_bytes(
             std::fs::read(&modules)?.as_slice(),
             modules,
+            &Default::default(),
         )?)
     }
 
@@ -213,13 +214,19 @@ mod update {
     fn valid_in_overrides() -> crate::Result {
         let mut module = submodule("[submodule.a]\n update = merge");
         let repo_config = gix_config::File::from_str("[submodule.a]\n update = !dangerous")?;
+        let prev_names = module.names().map(ToOwned::to_owned).collect::<Vec<_>>();
         module.append_submodule_overrides(&repo_config);
 
         assert_eq!(
             module.update("a".into())?.expect("present"),
             Update::Command("dangerous".into()),
             "overridden values are picked up and make commands possible - these are local"
         );
+        assert_eq!(
+            module.names().map(ToOwned::to_owned).collect::<Vec<_>>(),
+            prev_names,
+            "Appending more configuration sections doesn't affect name listing"
+        );
         Ok(())
     }