From 5d611fa66b0e7bf79072de93b20097e2a89fba19 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 6 Nov 2025 08:57:14 -0500 Subject: [PATCH 1/4] test(config): wrong merge order for non-mergeable list This show the problematic merge order of non-mergeable list --- tests/testsuite/config.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index eb0b50a3cb0..2589c3d48f3 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -2209,6 +2209,21 @@ credential-provider = ['c', 'd'] .unwrap(); assert_eq!(provider.path.raw_value(), "c"); assert_eq!(provider.args, ["d"]); + + let cli_arg = "registries.example.credential-provider=['cli', 'cli-arg']"; + let gctx = GlobalContextBuilder::new() + .config_arg(cli_arg) + .cwd("foo") + .build(); + let provider = gctx + .get::>(&format!("registries.example")) + .unwrap() + .unwrap() + .credential_provider + .unwrap(); + // expect: no merge happens; config CLI takes precedence + assert_eq!(provider.path.raw_value(), "c"); + assert_eq!(provider.args, ["d", "cli", "cli-arg"]); } #[cargo_test] From 8b3728c7fa25797fc512f54b7b4b28768b71f482 Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 6 Nov 2025 09:10:32 -0500 Subject: [PATCH 2/4] fix: non-mergeable list from config cli merge the same way Before we iterated key values directly for config cli values, so top level table name of config key is never tracked. And that resulted in `registries.example.credential-provider` becoming `example.credential-provider`, making `is_nonmergable_list` return false. This fix replaces the manual traversal with a direct `ConfigValue::merge` call. Now we unify how config values merge, regardless theird sources. --- src/cargo/util/context/mod.rs | 44 ++++++++++++++++++------------- tests/testsuite/config.rs | 6 ++--- tests/testsuite/config_include.rs | 2 +- 3 files changed, 30 insertions(+), 22 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 6d7e93b9a59..e5901408803 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1546,24 +1546,18 @@ impl GlobalContext { /// Add config arguments passed on the command line. fn merge_cli_args(&mut self) -> CargoResult<()> { - let CV::Table(loaded_map, _def) = self.cli_args_as_table()? else { - unreachable!() - }; - let values = self.values_mut()?; - for (key, value) in loaded_map.into_iter() { - match values.entry(key) { - Vacant(entry) => { - entry.insert(value); - } - Occupied(mut entry) => entry.get_mut().merge(value, true).with_context(|| { - format!( - "failed to merge --config key `{}` into `{}`", - entry.key(), - entry.get().definition(), - ) - })?, - }; - } + let cv_from_cli = self.cli_args_as_table()?; + assert!(cv_from_cli.is_table(), "cv from CLI must be a table"); + + let root_cv = mem::take(self.values_mut()?); + // This definition path is ignored, this is just a temporary container + // representing the entire file. + let mut root_cv = CV::Table(root_cv, Definition::Path(PathBuf::from("."))); + root_cv.merge(cv_from_cli, true)?; + + // Put it back to gctx + mem::swap(self.values_mut()?, root_cv.table_mut("")?.0); + Ok(()) } @@ -2299,6 +2293,20 @@ impl ConfigValue { } } + pub fn table_mut( + &mut self, + key: &str, + ) -> CargoResult<(&mut HashMap, &mut Definition)> { + match self { + CV::Table(table, def) => Ok((table, def)), + _ => self.expected("table", key), + } + } + + pub fn is_table(&self) -> bool { + matches!(self, CV::Table(_table, _def)) + } + pub fn string_list(&self, key: &str) -> CargoResult> { match self { CV::List(list, _) => list diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 2589c3d48f3..fa3f1293383 100644 --- a/tests/testsuite/config.rs +++ b/tests/testsuite/config.rs @@ -652,7 +652,7 @@ Caused by: assert_error( gctx.unwrap_err(), str![[r#" -failed to merge --config key `a` into `[ROOT]/.cargo/config.toml` +failed to merge key `a` between [ROOT]/.cargo/config.toml and --config cli option Caused by: failed to merge config value from `--config cli option` into `[ROOT]/.cargo/config.toml`: expected boolean, but found array @@ -2222,8 +2222,8 @@ credential-provider = ['c', 'd'] .credential_provider .unwrap(); // expect: no merge happens; config CLI takes precedence - assert_eq!(provider.path.raw_value(), "c"); - assert_eq!(provider.args, ["d", "cli", "cli-arg"]); + assert_eq!(provider.path.raw_value(), "cli"); + assert_eq!(provider.args, ["cli-arg"]); } #[cargo_test] diff --git a/tests/testsuite/config_include.rs b/tests/testsuite/config_include.rs index 444f7e86ba4..06e71f2901d 100644 --- a/tests/testsuite/config_include.rs +++ b/tests/testsuite/config_include.rs @@ -459,7 +459,7 @@ fn cli_merge_failed() { assert_error( gctx.unwrap_err(), str![[r#" -failed to merge --config key `foo` into `[ROOT]/.cargo/config.toml` +failed to merge key `foo` between [ROOT]/.cargo/config.toml and [ROOT]/.cargo/other.toml Caused by: failed to merge config value from `[ROOT]/.cargo/other.toml` into `[ROOT]/.cargo/config.toml`: expected array, but found string From fc43bf8100e32db9d4ba16db95bd644a4fe9f20a Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 6 Nov 2025 09:32:12 -0500 Subject: [PATCH 3/4] refactor: use built-in definition for root cv container The root config value container isn't from any external source, so its definition should be built-in. --- src/cargo/util/context/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index e5901408803..731d3c29c1b 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1286,9 +1286,9 @@ impl GlobalContext { /// Start a config file discovery from a path and merges all config values found. fn load_values_from(&self, path: &Path) -> CargoResult> { - // This definition path is ignored, this is just a temporary container - // representing the entire file. - let mut cfg = CV::Table(HashMap::new(), Definition::Path(PathBuf::from("."))); + // The root config value container isn't from any external source, + // so its definition should be built-in. + let mut cfg = CV::Table(HashMap::new(), Definition::BuiltIn); let home = self.home_path.clone().into_path_unlocked(); self.walk_tree(path, &home, |path| { @@ -1550,9 +1550,9 @@ impl GlobalContext { assert!(cv_from_cli.is_table(), "cv from CLI must be a table"); let root_cv = mem::take(self.values_mut()?); - // This definition path is ignored, this is just a temporary container - // representing the entire file. - let mut root_cv = CV::Table(root_cv, Definition::Path(PathBuf::from("."))); + // The root config value container isn't from any external source, + // so its definition should be built-in. + let mut root_cv = CV::Table(root_cv, Definition::BuiltIn); root_cv.merge(cv_from_cli, true)?; // Put it back to gctx From 9c8521d363aeef1f8b0d6d39931790a21faee3be Mon Sep 17 00:00:00 2001 From: Weihang Lo Date: Thu, 6 Nov 2025 09:35:50 -0500 Subject: [PATCH 4/4] refactor: reuse `ConfigValue::table_mut` when expecting a table --- src/cargo/util/context/mod.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/cargo/util/context/mod.rs b/src/cargo/util/context/mod.rs index 731d3c29c1b..f2329b38cd4 100644 --- a/src/cargo/util/context/mod.rs +++ b/src/cargo/util/context/mod.rs @@ -1704,9 +1704,7 @@ impl GlobalContext { let mut value = self.load_file(&credentials)?; // Backwards compatibility for old `.cargo/credentials` layout. { - let CV::Table(ref mut value_map, ref def) = value else { - unreachable!(); - }; + let (value_map, def) = value.table_mut("")?; if let Some(token) = value_map.remove("token") { if let Vacant(entry) = value_map.entry("registry".into()) {