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 nabijaczleweli/cargo-update#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:

cargo/src/cargo/ops/cargo_install.rs

Lines 359 to 362 in 695bc5e

	list.v1
	.entry(pkg.package_id().clone())
	.or_insert_with(BTreeSet::new)
	.insert(bin.to_string());

or_insert_with(BTreeSet::new) above uses std::cmp::Ord.
Until now it seems fine but implementation of Ord doesn't compare git hashes:

cargo/src/cargo/core/source/source_id.rs

Lines 451 to 453 in 695bc5e

	(&Kind::Git(ref ref1), &Kind::Git(ref ref2)) => {
	(ref1, &self.canonical_url).cmp(&(ref2, &other.canonical_url))
	}

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: mati865@89a2404

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?

[mati865]

@alexcrichton we want to replace Entry or key because it contains all the metadata. Current (bugged) code does it by removing names of replaced binaries from the set and inserting new Entry with proper name in the set. Later on entries with empty sets are dropped.
Current implementation of Eq, Hash, Ord won't let us add new entry unless name, version or url is different (git rev doesn't matter).

Replacing entries while keeping values would be great but it's not stable yet. Tracking issue: rust-lang/rust#44286 not available for BTreeMap.
As workaround we could clone old values, remove entry and then insert entry with old values. I'll try that tomorrow.

[alexcrichton]

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