Auto merge of #4839 - sfackler:consistent-config, r=withoutboats

Make .cargo/credentials a subset of .cargo/config

Previously, .cargo/credentials looked like

```toml
token = "..."

[my-registry]
token = "..."
```

And was simply merged into the `registry` block of .cargo/config. This
meant that custom registry tokens were under
`registry.my-registry.token` rather than `registries.my-registry.token`
which is where the index was located, and that you couldn't have a
custom registry named `token` or it'd conflict with the token for the
default registry.

This commit changes things such that .cargo/credentials has the same
layout as .cargo/config, but only contains token values. For backwards
compatibility, we move `token` to `registry.token` when parsing.

Author: bors

src/cargo/ops/registry.rs

@@ -220,7 +220,7 @@ pub fn registry_configuration(config: &Config,
     let (index, token) = match registry {
         Some(registry) => {
             (Some(config.get_registry_index(&registry)?.to_string()),
-             config.get_string(&format!("registry.{}.token", registry))?.map(|p| p.val))
+             config.get_string(&format!("registries.{}.token", registry))?.map(|p| p.val))
         }
         None => {
             // Checking out for default index and token

src/cargo/util/config.rs

@@ -573,30 +573,31 @@ impl Config {
             format!("could not parse TOML configuration in `{}`", credentials.display())
         })?;
 
-        let value = CV::from_toml(&credentials, toml).chain_err(|| {
+        let mut value = CV::from_toml(&credentials, toml).chain_err(|| {
             format!("failed to load TOML configuration from `{}`", credentials.display())
         })?;
 
-        let cfg = match *cfg {
-            CV::Table(ref mut map, _) => map,
-            _ => unreachable!(),
-        };
-
-        let registry = cfg.entry("registry".into())
-                          .or_insert_with(|| CV::Table(HashMap::new(), PathBuf::from(".")));
-
-        match (registry, value) {
-            (&mut CV::Table(ref mut old, _), CV::Table(ref mut new, _)) => {
-                // Take ownership of `new` by swapping it with an empty hashmap, so we can move
-                // into an iterator.
-                let new = mem::replace(new, HashMap::new());
-                for (key, value) in new {
-                    old.insert(key, value);
+        // backwards compatibility for old .cargo/credentials layout
+        {
+            let value = match value {
+                CV::Table(ref mut value, _) => value,
+                _ => unreachable!(),
+            };
+
+            if let Some(token) = value.remove("token") {
+                if let Vacant(entry) = value.entry("registry".into()) {
+                    let mut map = HashMap::new();
+                    map.insert("token".into(), token);
+                    let table = CV::Table(map, PathBuf::from("."));
+                    entry.insert(table);
                 }
             }
-            _ => unreachable!(),
         }
 
+        // we want value to override cfg, so swap these
+        mem::swap(cfg, &mut value);
+        cfg.merge(value)?;
+
         Ok(())
     }
 
@@ -910,13 +911,16 @@ pub fn save_credentials(cfg: &Config,
     let (key, value) = {
         let key = "token".to_string();
         let value = ConfigValue::String(token, file.path().to_path_buf());
+        let mut map = HashMap::new();
+        map.insert(key, value);
+        let table = CV::Table(map, file.path().to_path_buf());
 
         if let Some(registry) = registry {
             let mut map = HashMap::new();
-            map.insert(key, value);
-            (registry, CV::Table(map, file.path().to_path_buf()))
+            map.insert(registry, table);
+            ("registries".into(), CV::Table(map, file.path().to_path_buf()))
         } else {
-            (key, value)
+            ("registry".into(), table)
         }
     };
 
@@ -926,6 +930,14 @@ pub fn save_credentials(cfg: &Config,
     })?;
 
     let mut toml = cargo_toml::parse(&contents, file.path(), cfg)?;
+
+    // move the old token location to the new one
+    if let Some(token) = toml.as_table_mut().unwrap().remove("token") {
+        let mut map = HashMap::new();
+        map.insert("token".to_string(), token);
+        toml.as_table_mut().unwrap().insert("registry".into(), map.into());
+    }
+
     toml.as_table_mut()
         .unwrap()
         .insert(key, value.into_toml());

tests/cargotest/support/publish.rs

@@ -12,7 +12,7 @@ pub fn setup() -> Repository {
     t!(fs::create_dir_all(config.parent().unwrap()));
     t!(t!(File::create(&config)).write_all(format!(r#"
         [registry]
-            token = "api-token"
+        token = "api-token"
 
         [registries.alternative]
         index = "{registry}"
@@ -21,7 +21,7 @@ pub fn setup() -> Repository {
     let credentials = paths::root().join("home/.cargo/credentials");
     t!(fs::create_dir_all(credentials.parent().unwrap()));
     t!(t!(File::create(&credentials)).write_all(br#"
-        [alternative]
+        [registries.alternative]
         token = "api-token"
     "#));
 

tests/login.rs

@@ -53,7 +53,9 @@ fn check_token(expected_token: &str, registry: Option<&str>) -> bool {
         // A registry has been provided, so check that the token exists in a
         // table for the registry.
         (Some(registry), toml::Value::Table(table)) => {
-            table.get(registry).and_then(|registry_table| {
+            table.get("registries")
+                    .and_then(|registries_table| registries_table.get(registry))
+                    .and_then(|registry_table| {
                 match registry_table.get("token") {
                     Some(&toml::Value::String(ref token)) => Some(token.as_str().to_string()),
                     _ => None,
@@ -62,7 +64,9 @@ fn check_token(expected_token: &str, registry: Option<&str>) -> bool {
         },
         // There is no registry provided, so check the global token instead.
         (None, toml::Value::Table(table)) => {
-            table.get("token").and_then(|v| {
+            table.get("registry")
+                    .and_then(|registry_table| registry_table.get("token"))
+                    .and_then(|v| {
                 match v {
                     &toml::Value::String(ref token) => Some(token.as_str().to_string()),
                     _ => None,

tests/publish.rs

@@ -4,7 +4,7 @@ extern crate hamcrest;
 extern crate tar;
 
 use std::io::prelude::*;
-use std::fs::File;
+use std::fs::{self, File};
 use std::io::SeekFrom;
 
 use cargotest::ChannelChanger;
@@ -70,6 +70,72 @@ See [..]
     }
 }
 
+#[test]
+fn old_token_location() {
+    publish::setup();
+
+    // publish::setup puts a token in this file.
+    fs::remove_file(paths::root().join(".cargo/config")).unwrap();
+
+    let credentials = paths::root().join("home/.cargo/credentials");
+    File::create(credentials)
+        .unwrap()
+        .write_all(br#"
+            token = "api-token"
+        "#)
+        .unwrap();
+
+    let p = project("foo")
+        .file("Cargo.toml", r#"
+            [project]
+            name = "foo"
+            version = "0.0.1"
+            authors = []
+            license = "MIT"
+            description = "foo"
+        "#)
+        .file("src/main.rs", "fn main() {}")
+        .build();
+
+    assert_that(p.cargo("publish").arg("--no-verify")
+                 .arg("--index").arg(publish::registry().to_string()),
+                execs().with_status(0).with_stderr(&format!("\
+[UPDATING] registry `{reg}`
+[WARNING] manifest has no documentation, [..]
+See [..]
+[PACKAGING] foo v0.0.1 ({dir})
+[UPLOADING] foo v0.0.1 ({dir})
+",
+        dir = p.url(),
+        reg = publish::registry())));
+
+    let mut f = File::open(&publish::upload_path().join("api/v1/crates/new")).unwrap();
+    // Skip the metadata payload and the size of the tarball
+    let mut sz = [0; 4];
+    assert_eq!(f.read(&mut sz).unwrap(), 4);
+    let sz = ((sz[0] as u32) <<  0) |
+             ((sz[1] as u32) <<  8) |
+             ((sz[2] as u32) << 16) |
+             ((sz[3] as u32) << 24);
+    f.seek(SeekFrom::Current(sz as i64 + 4)).unwrap();
+
+    // Verify the tarball
+    let mut rdr = GzDecoder::new(f);
+    assert_eq!(rdr.header().unwrap().filename().unwrap(), b"foo-0.0.1.crate");
+    let mut contents = Vec::new();
+    rdr.read_to_end(&mut contents).unwrap();
+    let mut ar = Archive::new(&contents[..]);
+    for file in ar.entries().unwrap() {
+        let file = file.unwrap();
+        let fname = file.header().path_bytes();
+        let fname = &*fname;
+        assert!(fname == b"foo-0.0.1/Cargo.toml" ||
+                fname == b"foo-0.0.1/Cargo.toml.orig" ||
+                fname == b"foo-0.0.1/src/main.rs",
+                "unexpected filename: {:?}", file.header().path());
+    }
+}
+
 // TODO: Deprecated
 // remove once it has been decided --host can be removed
 #[test]