apply transport 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.

Author: Byron

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::apply(&mut config, &repo.options.config_overrides, git_config::Source::Api)
-        .expect("applied once and can be applied again");
+    crate::config::overrides::apply(
+        &mut config,
+        &repo.options.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");

git-repository/src/config/cache/init.rs

@@ -4,6 +4,8 @@ use crate::{
     config::{cache::util::ApplyLeniency, Cache},
     repository,
 };
+use git_config::File;
+use git_sec::Permission;
 
 /// Initialization
 impl Cache {
@@ -114,8 +116,9 @@ impl Cache {
                 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)?;
+                crate::config::overrides::apply(&mut globals, config_overrides, git_config::Source::Api, |_| None)?;
             }
+            apply_environment_overrides(&mut globals, *git_prefix)?;
             globals
         };
 
@@ -194,3 +197,32 @@ impl Cache {
         Ok(())
     }
 }
+
+fn apply_environment_overrides(config: &mut File<'static>, git_prefix: Permission) -> Result<(), Error> {
+    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) in [
+        ("GIT_HTTP_LOW_SPEED_LIMIT", "lowSpeedLimit"),
+        ("GIT_HTTP_LOW_SPEED_TIME", "lowSpeedTime"),
+        ("GIT_HTTP_USER_AGENT", "userAgent"),
+    ] {
+        if let Some(value) = git_prefix
+            .check_opt(var)
+            .and_then(std::env::var_os)
+            .and_then(|val| git_path::os_string_into_bstring(val).ok())
+        {
+            section.push_with_comment(
+                key.try_into().expect("statically known to be valid"),
+                Some(value.as_ref()),
+                format!("from {var}").as_str(),
+            );
+        }
+    }
+
+    if !section.is_void() {
+        config.append(env_override);
+    }
+    Ok(())
+}

git-repository/src/config/overrides.rs

@@ -21,6 +21,7 @@ pub(crate) fn apply(
     config: &mut git_config::File<'static>,
     values: impl IntoIterator<Item = impl AsRef<BStr>>,
     source: git_config::Source,
+    mut make_comment: impl FnMut(&BStr) -> Option<BString>,
 ) -> 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 apply(
         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(())

git-repository/src/config/snapshot/access.rs

@@ -94,7 +94,9 @@ impl<'repo> SnapshotMut<'repo> {
         &mut self,
         values: impl IntoIterator<Item = impl AsRef<BStr>>,
     ) -> Result<&mut Self, crate::config::overrides::Error> {
-        crate::config::overrides::apply(&mut self.config, values, git_config::Source::Cli)?;
+        crate::config::overrides::apply(&mut self.config, values, git_config::Source::Cli, |v| {
+            Some(format!("-c {v}").into())
+        })?;
         Ok(self)
     }
     /// Apply all changes made to this instance.

git-repository/src/open.rs

@@ -320,7 +320,6 @@ impl ThreadSafeRepository {
     // 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<PathBuf>,

git-repository/src/repository/config/transport.rs

@@ -88,13 +88,8 @@ impl crate::Repository {
                     where
                         T: TryFrom<i64>,
                     {
-                        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,7 +161,7 @@ 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())))

git-repository/tests/repository/open.rs

@@ -44,3 +44,62 @@ mod submodules {
         .to_thread_local())
     }
 }
+
+mod with_overrides {
+    use crate::util::named_subrepo_opts;
+    use git::bstr::ByteSlice;
+    use git_repository as git;
+    use git_sec::Permission;
+    use git_testtools::Env;
+    use serial_test::serial;
+
+    #[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");
+        let mut opts = git::open::Options::isolated().config_overrides([
+            "http.userAgent=agent-from-api",
+            "http.lowSpeedLimit=2",
+            "http.lowSpeedTime=2",
+        ]);
+        opts.permissions.env.git_prefix = Permission::Allow;
+        let mut repo = named_subrepo_opts("make_config_repos.sh", "http-config", opts)?;
+        repo.config_snapshot_mut().apply_cli_overrides([
+            "http.userAgent=agent-from-cli",
+            "http.lowSpeedLimit=3",
+            "http.lowSpeedTime=3",
+        ])?;
+        let mut actual = Vec::<u8>::new();
+        for section in repo
+            .config_snapshot()
+            .plumbing()
+            .sections_by_name("http")
+            .expect("http configured")
+            .filter(|s| s.num_values() == 3)
+        {
+            section.write_to(&mut actual)?
+        }
+        assert_eq!(
+            actual.as_bstr(), "[http]\n\
+                               \tuserAgent = agent-from-api\n\
+                               \tlowSpeedLimit = 2\n\
+                               \tlowSpeedTime = 2\n\
+                               [http]\n\
+                               \tlowSpeedLimit = 1 # from GIT_HTTP_LOW_SPEED_LIMIT\n\
+                               \tlowSpeedTime = 1 # from GIT_HTTP_LOW_SPEED_TIME\n\
+                               \tuserAgent = agent from env # from GIT_HTTP_USER_AGENT\n\
+                               [http]\n\
+                               \tuserAgent = agent-from-cli # -c http.userAgent=agent-from-cli\n\
+                               \tlowSpeedLimit = 3 # -c http.lowSpeedLimit=3\n\
+                               \tlowSpeedTime = 3 # -c http.lowSpeedTime=3\n",
+            concat!(
+                "the order isn't exactly what we would want as env-to-config overrides should be last.",
+                "those who rely on it must use the `Source` of sections of values to prioritize themselves which isn't awesome"
+            )
+        );
+        Ok(())
+    }
+}

git-repository/tests/util/mod.rs

@@ -23,6 +23,11 @@ pub fn named_repo(name: &str) -> Result<Repository> {
     Ok(ThreadSafeRepository::open_opts(repo_path, restricted())?.to_thread_local())
 }
 
+pub fn named_subrepo_opts(fixture: &str, name: &str, opts: open::Options) -> Result<Repository> {
+    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()
 }

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<R: std::fmt::Debug>(&self, resource: R) -> Option<R> {
         match self {
             Permission::Allow => Some(resource),