fix: Sparse URLs in TomlLockfileSourceId

[secona]

What does this PR try to resolve?

Sparse registries have their IDs prefixed with their kind (sparse+). This PR fixes the current implementation of TomlLockfileSourceId where it incorrectly splits the kind and URL for sparse registries.

This change itself shouldn't affect cargo. It does, however, affect users of cargo-util-schemas, i.e. cargo plumbing commands. See crate-ci/cargo-plumbing#111

How to test and review this PR?

Verify how source IDs are made, especially their URLs.

[rustbot]

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ehuss]

Thanks! Would it be possible to add some tests around this?

[secona]

I've added some unit tests for this. Let me know if there are other changes that you'd like me to make.

[0xPoe]

Thanks!
I think this change makes sense for cargo-util-schemas. This bug didn’t affect Cargo itself because we handled it when constructing the source ID.

It would be great if we could introduce an end-to-end test to verify that Cargo itself correctly handles this prefix, but I think it’s fine to add that in a separate PR.

Maybe something like:

    let _registry = registry::RegistryBuilder::new()
        .http_index()
        .alternative()
        .build();

    Package::new("two-ver", "0.1.0").alternative(true).publish();
    Package::new("two-ver", "0.2.0").alternative(true).publish();
    let p = project()
        .file(
            "Cargo.toml",
            r#"
                [package]
                name = "foo"
                version = "0.1.0"
                edition = "2018"

                [dependencies]
                two-ver = { registry = 'alternative', version = "0.1.0" }
                two-ver2 = { registry = 'alternative', package = "two-ver", version = "0.2.0" }
            "#,
        )
        .file("src/lib.rs", "")
        .file("cratesio", "")
        .build();

    p.cargo("generate-lockfile").run();
    let lockfile = p.read_lockfile();
	// TODO: check source ID URL here.
    assert_eq!("", lockfile);

[secona]

I thought cargo already had one.

cargo/tests/testsuite/alt_registry.rs

Lines 1768 to 1816 in 1c714ca

	#[cargo_test]
	fn sparse_lockfile() {
	let _registry = registry::RegistryBuilder::new()
	.http_index()
	.alternative()
	.build();
	Package::new("foo", "0.1.0").alternative(true).publish();
	let p = project()
	.file(
	"Cargo.toml",
	r#"
	[project]
	name = "a"
	version = "0.5.0"
	authors = []
	edition = "2015"
	[dependencies]
	foo = { registry = 'alternative', version = '0.1.0'}
	"#,
	)
	.file("src/lib.rs", "")
	.build();
	p.cargo("generate-lockfile").run();
	assert_e2e().eq(
	&p.read_lockfile(),
	str![[r##"
	# This file is automatically @generated by Cargo.
	# It is not intended for manual editing.
	version = 4
	[[package]]
	name = "a"
	version = "0.5.0"
	dependencies = [
	"foo",
	]
	[[package]]
	name = "foo"
	version = "0.1.0"
	source = "sparse+http://127.0.0.1:[..]/index/"
	checksum = "458c1addb23fde7dfbca0410afdbcc0086f96197281ec304d9e0e10def3cb899"
	"##]],
	);
	}

That being said, this change is also a fix for the url field of TomlLockfileSourceId, which Cargo doesn't really use other than for sources ordering. The bug also doesn't affect sources ordering because we order by source kind first and then URLs. Cargo only cares about the source_str field which is used to make the SourceIds and for writing the lock file. The url field is mostly for users of cargo-util-schemas.

I don't think adding an end-to-end test is going to cover this problem.

[0xPoe]

I thought cargo already had one.

Good catch! I didn't find it last night.

I don't think adding an end-to-end test is going to cover this problem.

I am not saying it's gonna cover this issue from cargo-util-schema. That's also why we should add a test for Cargo itself in a separate PR.(The one you post in your comment)

Thank you! I think this PR is ready to go.