From 265e24e7e182e108864644909c09526983fcf5c7 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Tue, 10 Dec 2024 12:48:44 -0600 Subject: [PATCH 1/2] Show existing behavior --- tests/testsuite/config.rs | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index ddaa168eb97..04990024d88 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -9,6 +9,7 @@ use std::path::{Path, PathBuf}; use cargo::core::features::{GitFeatures, GitoxideFeatures}; use cargo::core::{PackageIdSpec, Shell}; +use cargo::util::auth::RegistryConfig; use cargo::util::context::{ self, Definition, GlobalContext, JobsConfig, SslVersionConfig, StringList, }; @@ -2163,3 +2164,34 @@ gitoxide = \"fetch\" unstable_flags.gitoxide == expect } } + +#[cargo_test] +fn nonmergable_lists() { + let root_path = paths::root().join(".cargo/config.toml"); + write_config_at( + &root_path, + "\ +[registries.example] +credential-provider = ['a', 'b'] +", + ); + + let foo_path = paths::root().join("foo/.cargo/config.toml"); + write_config_at( + &foo_path, + "\ +[registries.example] +credential-provider = ['c', 'd'] +", + ); + + let gctx = GlobalContextBuilder::new().cwd("foo").build(); + let provider = gctx + .get::>(&format!("registries.example")) + .unwrap() + .unwrap() + .credential_provider + .unwrap(); + assert_eq!(provider.path.raw_value(), "a"); + assert_eq!(provider.args, ["b", "c", "d"]); +} From d60db46956e982dc48b88a1edaa951cf96f89351 Mon Sep 17 00:00:00 2001 From: Arlo Siemsen Date: Tue, 10 Dec 2024 12:48:13 -0600 Subject: [PATCH 2/2] fix(config): Don't merge unmergable config --- src/cargo/util/context/de.rs | 16 +++----- src/cargo/util/context/key.rs | 10 +++++ src/cargo/util/context/mod.rs | 70 ++++++++++++++++++++-------------- src/cargo/util/context/path.rs | 4 +- tests/testsuite/config.rs | 4 +- 5 files changed, 62 insertions(+), 42 deletions(-) diff --git a/src/cargo/util/context/de.rs b/src/cargo/util/context/de.rs index 2073af59d12..fd149e7dbe5 100644 --- a/src/cargo/util/context/de.rs +++ b/src/cargo/util/context/de.rs @@ -154,17 +154,13 @@ impl<'de, 'gctx> de::Deserializer<'de> for Deserializer<'gctx> { where V: de::Visitor<'de>, { - let merge = if name == "StringList" { - true - } else if name == "UnmergedStringList" { - false + if name == "StringList" { + let vals = self.gctx.get_list_or_string(&self.key)?; + let vals: Vec = vals.into_iter().map(|vd| vd.0).collect(); + visitor.visit_newtype_struct(vals.into_deserializer()) } else { - return visitor.visit_newtype_struct(self); - }; - - let vals = self.gctx.get_list_or_string(&self.key, merge)?; - let vals: Vec = vals.into_iter().map(|vd| vd.0).collect(); - visitor.visit_newtype_struct(vals.into_deserializer()) + visitor.visit_newtype_struct(self) + } } fn deserialize_enum( diff --git a/src/cargo/util/context/key.rs b/src/cargo/util/context/key.rs index cd19011403b..66d12c83ccd 100644 --- a/src/cargo/util/context/key.rs +++ b/src/cargo/util/context/key.rs @@ -93,6 +93,16 @@ impl ConfigKey { pub fn is_root(&self) -> bool { self.parts.is_empty() } + + /// Returns whether or not the given key string matches this key. + /// Use * to match any key part. + pub fn matches(&self, pattern: &str) -> bool { + let mut parts = self.parts(); + pattern + .split('.') + .all(|pat| parts.next() == Some(pat) || pat == "*") + && parts.next().is_none() + } } impl fmt::Display for ConfigKey { diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 9824eff9017..ad8a772f499 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -736,7 +736,7 @@ impl GlobalContext { Ok(Some(CV::List(cv_list, cv_def))) } Some(cv) => { - // This can't assume StringList or UnmergedStringList. + // This can't assume StringList. // Return an error, which is the behavior of merging // multiple config.toml files with the same scenario. bail!( @@ -910,21 +910,9 @@ impl GlobalContext { } /// Helper for `StringList` type to get something that is a string or list. - fn get_list_or_string( - &self, - key: &ConfigKey, - merge: bool, - ) -> CargoResult> { + fn get_list_or_string(&self, key: &ConfigKey) -> CargoResult> { let mut res = Vec::new(); - if !merge { - self.get_env_list(key, &mut res)?; - - if !res.is_empty() { - return Ok(res); - } - } - match self.get_cv(key)? { Some(CV::List(val, _def)) => res.extend(val), Some(CV::String(val, def)) => { @@ -943,6 +931,7 @@ impl GlobalContext { } /// Internal method for getting an environment variable as a list. + /// If the key is a non-mergable list and a value is found in the environment, existing values are cleared. fn get_env_list( &self, key: &ConfigKey, @@ -953,6 +942,10 @@ impl GlobalContext { return Ok(()); }; + if is_nonmergable_list(&key) { + output.clear(); + } + let def = Definition::Environment(key.as_env_key().to_string()); if self.cli_unstable().advanced_env && env_val.starts_with('[') && env_val.ends_with(']') { // Parse an environment string as a TOML array. @@ -2227,13 +2220,31 @@ impl ConfigValue { /// /// Container and non-container types cannot be mixed. fn merge(&mut self, from: ConfigValue, force: bool) -> CargoResult<()> { + self.merge_helper(from, force, &mut ConfigKey::new()) + } + + fn merge_helper( + &mut self, + from: ConfigValue, + force: bool, + parts: &mut ConfigKey, + ) -> CargoResult<()> { + let is_higher_priority = from.definition().is_higher_priority(self.definition()); match (self, from) { (&mut CV::List(ref mut old, _), CV::List(ref mut new, _)) => { - if force { - old.append(new); + if is_nonmergable_list(&parts) { + // Use whichever list is higher priority. + if force || is_higher_priority { + mem::swap(new, old); + } } else { - new.append(old); - mem::swap(new, old); + // Merge the lists together. + if force { + old.append(new); + } else { + new.append(old); + mem::swap(new, old); + } } old.sort_by(|a, b| a.1.cmp(&b.1)); } @@ -2243,7 +2254,8 @@ impl ConfigValue { Occupied(mut entry) => { let new_def = value.definition().clone(); let entry = entry.get_mut(); - entry.merge(value, force).with_context(|| { + parts.push(&key); + entry.merge_helper(value, force, parts).with_context(|| { format!( "failed to merge key `{}` between \ {} and {}", @@ -2273,7 +2285,7 @@ impl ConfigValue { )); } (old, mut new) => { - if force || new.definition().is_higher_priority(old.definition()) { + if force || is_higher_priority { mem::swap(old, &mut new); } } @@ -2348,6 +2360,16 @@ impl ConfigValue { } } +/// List of which configuration lists cannot be merged. +/// Instead of merging, these the higher priority list replaces the lower priority list. +fn is_nonmergable_list(key: &ConfigKey) -> bool { + key.matches("registries.*.credential-provider") + || key.matches("target.*.runner") + || key.matches("host.runner") + || key.matches("credential-alias.*") + || key.matches("doc.browser") +} + pub fn homedir(cwd: &Path) -> Option { ::home::cargo_home_with_cwd(cwd).ok() } @@ -2916,14 +2938,6 @@ impl StringList { } } -/// Alternative to [`StringList`] that follows precedence rules, rather than merging config values with environment values, -/// -/// e.g. a string list found in the environment will be used instead of one in a config file. -/// -/// This is currently only used by [`PathAndArgs`] -#[derive(Debug, Deserialize)] -pub struct UnmergedStringList(Vec); - #[macro_export] macro_rules! __shell_print { ($config:expr, $which:ident, $newline:literal, $($arg:tt)*) => ({ diff --git a/src/cargo/util/context/path.rs b/src/cargo/util/context/path.rs index bf96b5846be..6f35ddc89b5 100644 --- a/src/cargo/util/context/path.rs +++ b/src/cargo/util/context/path.rs @@ -1,4 +1,4 @@ -use super::{GlobalContext, UnmergedStringList, Value}; +use super::{GlobalContext, StringList, Value}; use serde::{de::Error, Deserialize}; use std::path::PathBuf; @@ -64,7 +64,7 @@ impl<'de> serde::Deserialize<'de> for PathAndArgs { where D: serde::Deserializer<'de>, { - let vsl = Value::::deserialize(deserializer)?; + let vsl = Value::::deserialize(deserializer)?; let mut strings = vsl.val.0; if strings.is_empty() { return Err(D::Error::invalid_length(0, &"at least one element")); diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 04990024d88..0756ef198fc 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -2192,6 +2192,6 @@ credential-provider = ['c', 'd'] .unwrap() .credential_provider .unwrap(); - assert_eq!(provider.path.raw_value(), "a"); - assert_eq!(provider.args, ["b", "c", "d"]); + assert_eq!(provider.path.raw_value(), "c"); + assert_eq!(provider.args, ["d"]); }