Allow using 'config.toml' instead of just 'config' files.

Note that this change only makes 'config.toml' optional to use instead of 'config'. If both exist, we will print a warning and prefer 'config', since that would be the existing behavior if both existed.

We should also consider a separate change to make config.toml the default and update docs, etc.

Author: zachlute

src/cargo/util/config.rs

@@ -662,7 +662,7 @@ impl Config {
         let mut cfg = CV::Table(HashMap::new(), PathBuf::from("."));
         let home = self.home_path.clone().into_path_unlocked();
 
-        walk_tree(path, &home, |path| {
+        self.walk_tree(path, &home, |path| {
             let mut contents = String::new();
             let mut file = File::open(&path)?;
             file.read_to_string(&mut contents)
@@ -689,6 +689,61 @@ impl Config {
         }
     }
 
+    fn walk_tree<F>(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()>
+    where
+        F: FnMut(&Path) -> CargoResult<()>,
+    {
+        let mut stash: HashSet<PathBuf> = HashSet::new();
+
+        for current in paths::ancestors(pwd) {
+            let possible = current.join(".cargo").join("config");
+            let possible_with_extension = current.join(".cargo").join("config.toml");
+
+            // If both 'config' and 'config.toml' exist, we should use 'config'
+            // for backward compatibility, but we should warn the user.
+            if fs::metadata(&possible).is_ok() {
+                if fs::metadata(&possible_with_extension).is_ok() {
+                    self.shell().warn(format!(
+                        "Both `{}` and `{}` exist. Using `{}`",
+                        possible.display(),
+                        possible_with_extension.display(),
+                        possible.display()
+                    ))?;
+                }
+
+                walk(&possible)?;
+                stash.insert(possible);
+            } else if fs::metadata(&possible_with_extension).is_ok() {
+                walk(&possible_with_extension)?;
+                stash.insert(possible);
+            }
+        }
+
+        // Once we're done, also be sure to walk the home directory even if it's not
+        // in our history to be sure we pick up that standard location for
+        // information.
+        let config = home.join("config");
+        let config_with_extension = home.join("config.toml");
+        if !stash.contains(&config) && fs::metadata(&config).is_ok() {
+            if fs::metadata(&config_with_extension).is_ok() {
+                self.shell().warn(format!(
+                    "Both `{}` and `{}` exist. Using `{}`",
+                    config.display(),
+                    config_with_extension.display(),
+                    config.display()
+                ))?;
+            }
+
+            walk(&config)?;
+        } else if !stash.contains(&config_with_extension)
+            && fs::metadata(&config_with_extension).is_ok()
+        {
+            walk(&config_with_extension)?;
+        }
+
+        Ok(())
+    }
+
     /// Gets the index for a registry.
     pub fn get_registry_index(&self, registry: &str) -> CargoResult<Url> {
         validate_package_name(registry, "registry name", "")?;
@@ -1673,31 +1728,6 @@ pub fn homedir(cwd: &Path) -> Option<PathBuf> {
     ::home::cargo_home_with_cwd(cwd).ok()
 }
 
-fn walk_tree<F>(pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()>
-where
-    F: FnMut(&Path) -> CargoResult<()>,
-{
-    let mut stash: HashSet<PathBuf> = HashSet::new();
-
-    for current in paths::ancestors(pwd) {
-        let possible = current.join(".cargo").join("config");
-        if fs::metadata(&possible).is_ok() {
-            walk(&possible)?;
-            stash.insert(possible);
-        }
-    }
-
-    // Once we're done, also be sure to walk the home directory even if it's not
-    // in our history to be sure we pick up that standard location for
-    // information.
-    let config = home.join("config");
-    if !stash.contains(&config) && fs::metadata(&config).is_ok() {
-        walk(&config)?;
-    }
-
-    Ok(())
-}
-
 pub fn save_credentials(cfg: &Config, token: String, registry: Option<String>) -> CargoResult<()> {
     let mut file = {
         cfg.home_path.create_dir()?;

tests/testsuite/config.rs

@@ -48,6 +48,12 @@ fn write_config(config: &str) {
     fs::write(path, config).unwrap();
 }
 
+fn write_config_toml(config: &str) {
+    let path = paths::root().join(".cargo/config.toml");
+    fs::create_dir_all(path.parent().unwrap()).unwrap();
+    fs::write(path, config).unwrap();
+}
+
 fn new_config(env: &[(&str, &str)]) -> Config {
     enable_nightly_features(); // -Z advanced-env
     let output = Box::new(fs::File::create(paths::root().join("shell.out")).unwrap());
@@ -112,6 +118,57 @@ f1 = 123
     assert_eq!(s, S { f1: Some(456) });
 }
 
+#[cargo_test]
+fn config_works_with_extension() {
+    write_config_toml(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    let config = new_config(&[]);
+
+    assert_eq!(config.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+}
+
+#[cargo_test]
+fn config_ambiguous_filename() {
+    write_config(
+        "\
+[foo]
+f1 = 1
+",
+    );
+
+    write_config_toml(
+        "\
+[foo]
+f1 = 2
+",
+    );
+
+    let config = new_config(&[]);
+
+    // It should use the value from the one without the extension for
+    // backwards compatibility.
+    assert_eq!(config.get::<Option<i32>>("foo.f1").unwrap(), Some(1));
+
+    // But it also should have warned.
+    drop(config); // Paranoid about flushing the file.
+    let path = paths::root().join("shell.out");
+    let output = fs::read_to_string(path).unwrap();
+    let expected = "\
+warning: Both `[..]/.cargo/config` and `[..]/.cargo/config.toml` exist. Using `[..]/.cargo/config`
+";
+    if !lines_match(expected, &output) {
+        panic!(
+            "Did not find expected:\n{}\nActual error:\n{}\n",
+            expected, output
+        );
+    }
+}
+
 #[cargo_test]
 fn config_unused_fields() {
     write_config(