cargo update SPEC broken if branch name contains plus sign

[kaspar030]

Problem

I have a crates.io patch pointing to a git repo and branch, and the branch name contains a plus sign.

That plus sign seems to break cargo update MY_DEP: (cargo update works fine)

urlplussign-bin on  main took 1s50ms 
❯ cargo update
    Updating git repository `https://github.com/kaspar030/urlplussign-lib`
    Updating crates.io index
     Locking 1 package to latest compatible version
    Removing urlplussign-lib v0.1.0 (https://github.com/kaspar030/urlplussign-lib?branch=main plus#2ff09b40)
      Adding urlplussign-lib v0.1.0 (https://github.com/kaspar030/urlplussign-lib?branch=main+plus#2ff09b40)
urlplussign-bin on  main took 1s4ms 
❯ cargo update urlplussign-lib
    Updating git repository `https://github.com/kaspar030/urlplussign-lib`
error: Unable to update https://github.com/kaspar030/urlplussign-lib?branch=main plus

Caused by:
  failed to fetch into: /home/kaspar/.cargo/git/db/urlplussign-lib-a84c539888aebb72

Caused by:
  '+refs/heads/main plus:refs/remotes/origin/main plus' is not a valid refspec.; class=Invalid (3)

Steps

I've built a minimal reproducer repo:

1. git clone https://github.com/kaspar030/urlplussign-bin
2. cd urlplussign-bin
3. cargo update urlplussign-lib # fails
4. cargo update # succeeds

Possible Solution(s)

No response

Notes

The plain cargo update output looks weird, too:

    Removing urlplussign-lib v0.1.0 (https://github.com/kaspar030/urlplussign-lib?branch=main plus#2ff09b40)
      Adding urlplussign-lib v0.1.0 (https://github.com/kaspar030/urlplussign-lib?branch=main+plus#2ff09b40)

Note how it is Removing w/o plus sign, adding with plus sign, for the same branch.

Version

❯ cargo version --verbose
cargo 1.82.0 (8f40fc59f 2024-08-21)
release: 1.82.0
commit-hash: 8f40fc59fb0c8df91c97405785197f3c630304ea
commit-date: 2024-08-21
host: x86_64-unknown-linux-gnu
libgit2: 1.8.1 (sys:0.19.0 vendored)
libcurl: 8.9.0-DEV (sys:0.4.74+curl-8.9.0 vendored ssl:OpenSSL/1.1.1w)
ssl: OpenSSL 1.1.1w  11 Sep 2023
os: Arch Linux Rolling Release [64-bit]

[ehuss]

I believe this has already been fixed with the v4 lockfile format. v4 will be the default in Rust 1.83. The v4 format is supported back to Rust 1.78.

You can update your lockfile to v4 by deleting it and rebuilding it with the beta or nightly toolchain. Or if you are adventurous, just edit the Cargo.lock file and change the version to 4 and run cargo update.

[epage]

I just confirmed that this is related to the <=v3 lockfile formats.

[epage]

v4 lockfile format will be the default (when compatible with your MSRV) as of 1.83, see #14595.

The original issue was #11085 which was fixed in #12280 and stabilized in #12852

[weihanglo]

This also happens on older versions of Cargo like 1.68.0.

This seems like a corner case between updating a selected package. When url crate read a package source, it always URL-decodes the source field (no way to make it not to). Therefore, Cargo always gets SourceId with branch main plus. I am not 100% sure the actual cause of this, but my gut feeling is that we might need a custom URL deserialization other than relying on url crate to fix this, and that means all structs, from EncodableResolve, EncodableDependency, EncodablePackageId and EncodableSourceId needs some adjustments.

[weihanglo]

BTW the presence of [patch] is not necessary to reproduce. Simply with some special characters in a branch name would cause the failure.

[package]
name = "foo"
edition = "2021"

[dependencies]
empty-library = { git = "https://github.com/weihanglo/empty-library.git", branch = "中文branch+cool%/&|@#bba41cab#123" }

[weihanglo]

but my gut feeling is that we might need a custom URL deserialization other than relying on url crate to fix this, and that means all structs, from EncodableResolve, EncodableDependency, EncodablePackageId and EncodableSourceId needs some adjustments.

Given the complexity (it doesn't seem hard but would need a bunch of extra code), I am not sure if it is worth a fix.

[weihanglo]

While this is a nasty papercut, I propose this as a wont-fix and close this for reasons below

• Git dependencies with branch/rev/tag with + or any other URL encodable characters are already broken in one way or the other.
• We already have lockfile v4 as a general fix.
• The fix from the user side is relatively easy — change their branch name.
• For this issue specifically, update can be done by removing and re-adding those dependencies.