From c530720b294fe4381b706954fdba6c14667d31b4 Mon Sep 17 00:00:00 2001 From: Zach Lute Date: Sat, 24 Aug 2019 12:43:15 -0700 Subject: [PATCH] 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. --- src/cargo/util/config.rs | 82 ++++++++++++++++++++++++++------------- tests/testsuite/config.rs | 57 +++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 26 deletions(-) diff --git a/src/cargo/util/config.rs b/src/cargo/util/config.rs index fa7b2b2bcb0..a7659288bb8 100644 --- a/src/cargo/util/config.rs +++ b/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(&self, pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()> + where + F: FnMut(&Path) -> CargoResult<()>, + { + let mut stash: HashSet = 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 { validate_package_name(registry, "registry name", "")?; @@ -1673,31 +1728,6 @@ pub fn homedir(cwd: &Path) -> Option { ::home::cargo_home_with_cwd(cwd).ok() } -fn walk_tree(pwd: &Path, home: &Path, mut walk: F) -> CargoResult<()> -where - F: FnMut(&Path) -> CargoResult<()>, -{ - let mut stash: HashSet = 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) -> CargoResult<()> { let mut file = { cfg.home_path.create_dir()?; diff --git a/tests/testsuite/config.rs b/tests/testsuite/config.rs index 5f3cd76c38b..8f7549c4309 100644 --- a/tests/testsuite/config.rs +++ b/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::>("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::>("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(