Auto merge of rust-lang#11756 - arlosi:sparse-uninstall, r=ehuss

Fix Cargo removing the sparse+ prefix from sparse URLs in .crates.toml

The URL associated with a `SourceId` for a sparse registry includes the `sparse+` prefix in the URL to differentiate from git (non-sparse) registries.

`SourceId::from_url` was not including the `sparse+` prefix of sparse registry URLs on construction, which caused roundtrips of `as_url` and `from_url` to fail by losing the prefix.

Fixes rust-lang#11751 by:
* Including the prefix in the URL
* Adding a test that verifies round-trip behavior for sparse `SourceId`s
* Modifying `CanonicalUrl` so it no longer considers `sparse+` and non-`sparse+` URLs to be equivalent

Author: bors

src/cargo/core/source/source_id.rs

@@ -81,6 +81,14 @@ impl SourceId {
     ///
     /// The canonical url will be calculated, but the precise field will not
     fn new(kind: SourceKind, url: Url, name: Option<&str>) -> CargoResult<SourceId> {
+        if kind == SourceKind::SparseRegistry {
+            // Sparse URLs are different because they store the kind prefix (sparse+)
+            // in the URL. This is because the prefix is necessary to differentiate
+            // from regular registries (git-based). The sparse+ prefix is included
+            // everywhere, including user-facing locations such as the `config.toml`
+            // file that defines the registry, or whenever Cargo displays it to the user.
+            assert!(url.as_str().starts_with("sparse+"));
+        }
         let source_id = SourceId::wrap(SourceIdInner {
             kind,
             canonical_url: CanonicalUrl::new(&url)?,
@@ -152,7 +160,7 @@ impl SourceId {
                     .with_precise(Some("locked".to_string())))
             }
             "sparse" => {
-                let url = url.into_url()?;
+                let url = string.into_url()?;
                 Ok(SourceId::new(SourceKind::SparseRegistry, url, None)?
                     .with_precise(Some("locked".to_string())))
             }
@@ -721,6 +729,7 @@ impl<'a> fmt::Display for SourceIdAsUrl<'a> {
                 ref url,
                 ..
             } => {
+                // Sparse registry URL already includes the `sparse+` prefix
                 write!(f, "{url}")
             }
             SourceIdInner {
@@ -864,4 +873,14 @@ mod tests {
         assert_eq!(gen_hash(source_id), 17459999773908528552);
         assert_eq!(crate::util::hex::short_hash(&source_id), "6568fe2c2fab5bfe");
     }
+
+    #[test]
+    fn serde_roundtrip() {
+        let url = "sparse+https://my-crates.io/".into_url().unwrap();
+        let source_id = SourceId::for_registry(&url).unwrap();
+        let formatted = format!("{}", source_id.as_url());
+        let deserialized = SourceId::from_url(&formatted).unwrap();
+        assert_eq!(formatted, "sparse+https://my-crates.io/");
+        assert_eq!(source_id, deserialized);
+    }
 }

src/cargo/util/canonical_url.rs

@@ -1,4 +1,4 @@
-use crate::util::{errors::CargoResult, IntoUrl};
+use crate::util::errors::CargoResult;
 use std::hash::{self, Hash};
 use url::Url;
 
@@ -56,17 +56,6 @@ impl CanonicalUrl {
             url.path_segments_mut().unwrap().pop().push(&last);
         }
 
-        // Ignore the protocol specifier (if any).
-        if url.scheme().starts_with("sparse+") {
-            // NOTE: it is illegal to use set_scheme to change sparse+http(s) to http(s).
-            url = url
-                .to_string()
-                .strip_prefix("sparse+")
-                .expect("we just found that prefix")
-                .into_url()
-                .expect("a valid url without a protocol specifier should still be valid");
-        }
-
         Ok(CanonicalUrl(url))
     }
 

tests/testsuite/install.rs

@@ -3,7 +3,7 @@
 use std::fs::{self, OpenOptions};
 use std::io::prelude::*;
 
-use cargo_test_support::cross_compile;
+use cargo_test_support::{cross_compile, compare};
 use cargo_test_support::git;
 use cargo_test_support::registry::{self, registry_path, Package};
 use cargo_test_support::{
@@ -2105,3 +2105,67 @@ fn no_auto_fix_note() {
         .run();
     assert_has_not_installed_exe(cargo_home(), "auto_fix");
 }
+
+#[cargo_test]
+fn sparse_install() {
+    // Checks for an issue where uninstalling something corrupted
+    // the SourceIds of sparse registries.
+    // See https://github.com/rust-lang/cargo/issues/11751
+    let _registry = registry::RegistryBuilder::new().http_index().build();
+
+    pkg("foo", "0.0.1");
+    pkg("bar", "0.0.1");
+
+    cargo_process("install foo --registry dummy-registry")
+        .with_stderr(
+            "\
+[UPDATING] `dummy-registry` index
+[DOWNLOADING] crates ...
+[DOWNLOADED] foo v0.0.1 (registry `dummy-registry`)
+[INSTALLING] foo v0.0.1 (registry `dummy-registry`)
+[UPDATING] `dummy-registry` index
+[COMPILING] foo v0.0.1 (registry `dummy-registry`)
+[FINISHED] release [optimized] target(s) in [..]
+[INSTALLING] [ROOT]/home/.cargo/bin/foo[EXE]
+[INSTALLED] package `foo v0.0.1 (registry `dummy-registry`)` (executable `foo[EXE]`)
+[WARNING] be sure to add `[..]` to your PATH to be able to run the installed binaries
+",
+        )
+        .run();
+    assert_has_installed_exe(cargo_home(), "foo");
+    let assert_v1 = |expected| {
+        let v1 = fs::read_to_string(paths::home().join(".cargo/.crates.toml")).unwrap();
+        compare::assert_match_exact(expected, &v1);
+    };
+    assert_v1(
+        r#"[v1]
+"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo[EXE]"]
+"#,
+    );
+    cargo_process("install bar").run();
+    assert_has_installed_exe(cargo_home(), "bar");
+    assert_v1(
+        r#"[v1]
+"bar 0.0.1 (registry+https://github.com/rust-lang/crates.io-index)" = ["bar[EXE]"]
+"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo[EXE]"]
+"#,
+    );
+
+    cargo_process("uninstall bar")
+        .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/bar[EXE]")
+        .run();
+    assert_has_not_installed_exe(cargo_home(), "bar");
+    assert_v1(
+        r#"[v1]
+"foo 0.0.1 (sparse+http://127.0.0.1:[..]/index/)" = ["foo[EXE]"]
+"#,
+    );
+    cargo_process("uninstall foo")
+        .with_stderr("[REMOVING] [CWD]/home/.cargo/bin/foo[EXE]")
+        .run();
+    assert_has_not_installed_exe(cargo_home(), "foo");
+    assert_v1(
+        r#"[v1]
+"#,
+    );
+}