non-mergeable list from env shouldn't take precendence over config-cli

[weihanglo]

Problem

Reproducible example

#[cargo_test]
fn nonmergeable_lists_precedence() {
    let foo_path = paths::root().join("foo/.cargo/config.toml");
    write_config_at(
        &foo_path,
        "\
[a]
b = ['chromium', 'config']
",
    );

    // config-cli takes precedence
    let config_arg = "doc.browser=['firefox', 'cli']";
    let gctx = GlobalContextBuilder::new()
        .cwd("foo")
        .config_arg(config_arg)
        .build();
    let provider = gctx
        .get::<Option<context::PathAndArgs>>("doc.browser")
        .unwrap()
        .unwrap();
    assert_eq!(provider.path.raw_value(), "firefox");
    assert_eq!(provider.args, ["cli"]);

    // config-cli should takes precedence, but env wins
    let gctx = GlobalContextBuilder::new()
        .cwd("foo")
        .env("CARGO_DOC_BROWSER", "servo env")
        .config_arg(config_arg)
        .build();
    let provider = gctx
        .get::<Option<context::PathAndArgs>>("doc.browser")
        .unwrap()
        .unwrap();
    assert_eq!(provider.path.raw_value(), "servo");
    assert_eq!(provider.args, ["env"]);
}

Possible Solution(s)

Probably something like this in get_env_list()?

+        let def = Definition::Environment(key.as_env_key().to_string());
+
         if is_nonmergeable_list(&key) {
-            output.clear();
+            debug_assert!(
+                output
+                    .windows(2)
+                    .all(|cvs| cvs[0].definition() == cvs[1].definition()),
+                "since the output is a non-mergeable list, it must have only one definition: {output:?}",
+            );
+
+            // We prefer to env config if no higher definition found (i.e, no `--config` CLI)
+            // Otherwise we should stop and use that instead.
+            if output.iter().all(|cv| &def >= cv.definition()) {
+                output.clear();
+            } else {
+                return Ok(());
+            }
         }

Version

latest master: 0f14d9d

I haven't checked if it was buggy since #15066 or it is a recent regression.

[epage]

I assume we have tests for other cases to ensure this is isolated to unmergeable lists?

[weihanglo]

I assume we have tests for other cases to ensure this is isolated to unmergeable lists?

Yes

cargo/tests/testsuite/config_cli.rs

Lines 26 to 27 in 0f14d9d

	fn cli_priority() {
	// Command line takes priority over files and env vars.

Was a bit unsure about nested, but it seems working:

diff --git a/tests/testsuite/config_cli.rs b/tests/testsuite/config_cli.rs
index 42b447704..df9fffd88 100644
--- a/tests/testsuite/config_cli.rs
+++ b/tests/testsuite/config_cli.rs
@@ -34,6 +34,8 @@ fn cli_priority() {
         [term]
         quiet = false
         verbose = false
+        [this.is.nested]
+        key = 'file'
         ",
     );
     let gctx = GlobalContextBuilder::new().build();
@@ -41,21 +43,25 @@ fn cli_priority() {
     assert_eq!(gctx.get::<String>("build.rustc").unwrap(), "file");
     assert_eq!(gctx.get::<bool>("term.quiet").unwrap(), false);
     assert_eq!(gctx.get::<bool>("term.verbose").unwrap(), false);
+    assert_eq!(gctx.get::<String>("this.is.nested.key").unwrap(), "file");
 
     let gctx = GlobalContextBuilder::new()
         .env("CARGO_BUILD_JOBS", "2")
         .env("CARGO_BUILD_RUSTC", "env")
         .env("CARGO_TERM_VERBOSE", "false")
         .env("CARGO_NET_GIT_FETCH_WITH_CLI", "false")
+        .env("CARGO_THIS_IS_NESTED_KEY", "env")
         .config_arg("build.jobs=1")
         .config_arg("build.rustc='cli'")
         .config_arg("term.verbose=true")
         .config_arg("net.git-fetch-with-cli=true")
+        .config_arg("this.is.nested.key='cli'")
         .build();
     assert_eq!(gctx.get::<i32>("build.jobs").unwrap(), 1);
     assert_eq!(gctx.get::<String>("build.rustc").unwrap(), "cli");
     assert_eq!(gctx.get::<bool>("term.verbose").unwrap(), true);
     assert_eq!(gctx.net_config().unwrap().git_fetch_with_cli, Some(true));
+    assert_eq!(gctx.get::<String>("this.is.nested.key").unwrap(), "cli");
 
     // Setting both term.verbose and term.quiet is invalid and is tested
     // in the run test suite.