Cargo: Cargo install -f --git doesn't update crate metadata

[vorner]

Hello

When I try to update a binary installed by cargo from git, it does so, but doesn't store the new git hash in cargo metadata.

This is in my .cargo/.crates.toml (just the relevant line):

"pgz 0.1.0 (git+https://github.com/vorner/pgz#f7ea6361ad6afb7eb2abb9b9b7abcc04bc3ad171)" = ["pgz"]

Now, I run cargo install:

$ cargo install -f --git https://github.com/vorner/pgz
    Updating git repository `https://github.com/vorner/pgz`
  Installing pgz v0.1.0 (https://github.com/vorner/pgz#9b109b57)
    Updating registry `https://github.com/rust-lang/crates.io-index`
   Compiling libc v0.2.31
   Compiling cc v1.0.0
   Compiling futures v0.1.16
   Compiling num_cpus v1.4.0
   Compiling futures-cpupool v0.1.6
   Compiling miniz-sys v0.1.10
   Compiling flate2 v0.2.20
   Compiling pgz v0.1.0 (https://github.com/vorner/pgz#9b109b57)
    Finished release [optimized] target(s) in 31.76 secs
   Replacing /home/vorner/.cargo/bin/pgz

As you can see, it shows a different hash. However, the line in the crates.toml file stays the same and the hash is not updated to reflect the newer version.

If it is relevant, both commits contain the same package version (v0.1.0).

[alexcrichton]

Does this end up causing problems down the road? Or is this just a when-manually-inspected-it-looks-wrong bug?

[vorner]

Yes, it does cause some minor annoyances. In https://github.com/nabijaczleweli/cargo-update/issues/51 I asked for the cargo-update to be able to update git-installed binaries. When it updates them, it is not reflected in the metadata. So if the upstream git moves forward, it then updates every time even when it is no longer necessary.

[alexcrichton]

Definitely makes sense yeah, shouldn't be too hard to ensure we update it!

[nabijaczleweli]

Tracked it down to roughly these install internals, even though I lack the motivation and the expertise to really fix that rn.

[mati865]

Related snippet:
https://github.com/rust-lang/cargo/blob/695bc5e488fb20fa6053c0ef00602f7701988323/src/cargo/ops/cargo_install.rs#L359-L362

or_insert_with(BTreeSet::new) above uses std::cmp::Ord.
Until now it seems fine but implementation of Ord doesn't compare git hashes:
https://github.com/rust-lang/cargo/blob/695bc5e488fb20fa6053c0ef00602f7701988323/src/cargo/core/source/source_id.rs#L451-L453
More human friendly form:

(Branch("master"), "https://github.com/rust-lang-nursery/rustfix").cmp(
    (Branch("master"), "https://github.com/rust-lang-nursery/rustfix"))

Obviously it returns Equal so or_insert_with(BTreeSet::new) returns old entry (with old git hash) instead of creating new one.

I see two possible solutions:

1. Change Ord implementation to compare git hashes as well.
   It causes minor breakage inside cargo that should be easy to fix, cargo test ouptut here.
   I can't tell about impact on other crates which depend on cargo.
2. Change install function to always replace metadata no matter if it changed or not.

Before opening PR I'd like to hear which option seems better.
CC @alexcrichton

[vorner]

I'm not him, but if I can offer my own humble opinion, option 2 looks both like less work and generally safer in the way the chance it breaks something else should be smaller. The only downside seems to be overwriting something when not strictly necessary, but this is not on any performance critical path, is it?

[alexcrichton]

@mati865 oh awesome thanks for tracking this down! Let's take the route of always replacing metadata, it's currently left out of Ord and Eq implementations but that definitely sounds like it's the bug here!

[mati865]

Looks like it's not that easy to force replacing metadata.
You cannot modify entry and Rust won't add duplicate to the *Map. So it has to be removed before inserting new one, like here: https://github.com/mati865/cargo/commit/89a2404b1fcf55891f3ef54788b20cc064c4c1e5

This approach however breaks 2 tests.

[alexcrichton]

@mati865 I think that patch may be removing too much by accident? I think that's deleting whole sets while we just want to replace keys, right?

[alexcrichton]

Ah maybe we can just manfacture an entirely new map, favoring new keys over old ones?