Auto merge of #9397 - alexcrichton:fix-hash, r=ehuss

bors · bors · commit 0ed318d182e4 · 2021-04-23T20:54:54.000Z
Restore crates.io's `SourceId` hash value to before This commit restores the hash value of the crates.io `SourceId` to what it was before #9384. In #9384 the enum variants of `SourceKind` were reordered which accidentally changed the hash value of the `SourceId` for crates.io. A change here means that users with a new version of Cargo will have to redownload the index and all crates, which is something that we strive to avoid forcing. In changing this, though, it required a manual implementation of `Ord` to still contain the actual fix from #9384 which is to sort `SourceKind` differently from how it's defined. I was curious as to why this was necessary since it wasn't ever necessary in the past and this led to an odd spelunking which turned up some interesting information. Turns out Rust 1.47 and after had a breaking change where Cargo would sort dependencies differently. This means that #9334 *could* have been opened up much earlier, but it never was. We ironically only saw an issue when we fixed this regression (although we didn't realize we were fixing a regression). This means that we are now permanently codifying the regression in Cargo.
diff --git a/src/cargo/core/source/source_id.rs b/src/cargo/core/source/source_id.rs
@@ -42,21 +42,18 @@ struct SourceIdInner {
 
 /// The possible kinds of code source. Along with `SourceIdInner`, this fully defines the
 /// source.
-#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq, Hash)]
 enum SourceKind {
-    // Note that the ordering here is important for how it affects the `Ord`
-    // implementation, notably how this affects the ordering of serialized
-    // packages into lock files.
-    /// A local path..
+    /// A git repository.
+    Git(GitReference),
+    /// A local path.
     Path,
     /// A remote registry.
     Registry,
     /// A local filesystem-based registry.
     LocalRegistry,
     /// A directory-based registry.
     Directory,
-    /// A git repository.
-    Git(GitReference),
 }
 
 /// Information to find a specific commit in a Git repository.
@@ -486,6 +483,110 @@ impl Hash for SourceId {
     }
 }
 
+// forward to `Ord`
+impl PartialOrd for SourceKind {
+    fn partial_cmp(&self, other: &SourceKind) -> Option<Ordering> {
+        Some(self.cmp(other))
+    }
+}
+
+// Note that this is specifically not derived on `SourceKind` although the
+// implementation here is very similar to what it might look like if it were
+// otherwise derived.
+//
+// The reason for this is somewhat obtuse. First of all the hash value of
+// `SourceKind` makes its way into `~/.cargo/registry/index/github.com-XXXX`
+// which means that changes to the hash means that all Rust users need to
+// redownload the crates.io index and all their crates. If possible we strive to
+// not change this to make this redownloading behavior happen as little as
+// possible. How is this connected to `Ord` you might ask? That's a good
+// question!
+//
+// Since the beginning of time `SourceKind` has had `#[derive(Hash)]`. It for
+// the longest time *also* derived the `Ord` and `PartialOrd` traits. In #8522,
+// however, the implementation of `Ord` changed. This handwritten implementation
+// forgot to sync itself with the originally derived implementation, namely
+// placing git dependencies as sorted after all other dependencies instead of
+// first as before.
+//
+// This regression in #8522 (Rust 1.47) went unnoticed. When we switched back
+// to a derived implementation in #9133 (Rust 1.52 beta) we only then ironically
+// saw an issue (#9334). In #9334 it was observed that stable Rust at the time
+// (1.51) was sorting git dependencies last, whereas Rust 1.52 beta would sort
+// git dependencies first. This is because the `PartialOrd` implementation in
+// 1.51 used #8522, the buggy implementation, which put git deps last. In 1.52
+// it was (unknowingly) restored to the pre-1.47 behavior with git dependencies
+// first.
+//
+// Because the breakage was only witnessed after the original breakage, this
+// trait implementation is preserving the "broken" behavior. Put a different way:
+//
+// * Rust pre-1.47 sorted git deps first.
+// * Rust 1.47 to Rust 1.51 sorted git deps last, a breaking change (#8522) that
+//   was never noticed.
+// * Rust 1.52 restored the pre-1.47 behavior (#9133, without knowing it did
+//   so), and breakage was witnessed by actual users due to difference with
+//   1.51.
+// * Rust 1.52 (the source as it lives now) was fixed to match the 1.47-1.51
+//   behavior (#9383), which is now considered intentionally breaking from the
+//   pre-1.47 behavior.
+//
+// Note that this was all discovered when Rust 1.53 was in nightly and 1.52 was
+// in beta. #9133 was in both beta and nightly at the time of discovery. For
+// 1.52 #9383 reverted #9133, meaning 1.52 is the same as 1.51. On nightly
+// (1.53) #9397 was created to fix the regression introduced by #9133 relative
+// to the current stable (1.51).
+//
+// That's all a long winded way of saying "it's wierd that git deps hash first
+// and are sorted last, but it's the way it is right now". The author of this
+// comment chose to handwrite the `Ord` implementation instead of the `Hash`
+// implementation, but it's only required that at most one of them is
+// hand-written because the other can be derived. Perhaps one day in
+// the future someone can figure out how to remove this behavior.
+impl Ord for SourceKind {
+    fn cmp(&self, other: &SourceKind) -> Ordering {
+        match (self, other) {
+            (SourceKind::Path, SourceKind::Path) => Ordering::Equal,
+            (SourceKind::Path, _) => Ordering::Less,
+            (_, SourceKind::Path) => Ordering::Greater,
+
+            (SourceKind::Registry, SourceKind::Registry) => Ordering::Equal,
+            (SourceKind::Registry, _) => Ordering::Less,
+            (_, SourceKind::Registry) => Ordering::Greater,
+
+            (SourceKind::LocalRegistry, SourceKind::LocalRegistry) => Ordering::Equal,
+            (SourceKind::LocalRegistry, _) => Ordering::Less,
+            (_, SourceKind::LocalRegistry) => Ordering::Greater,
+
+            (SourceKind::Directory, SourceKind::Directory) => Ordering::Equal,
+            (SourceKind::Directory, _) => Ordering::Less,
+            (_, SourceKind::Directory) => Ordering::Greater,
+
+            (SourceKind::Git(a), SourceKind::Git(b)) => a.cmp(b),
+        }
+    }
+}
+
+// This is a test that the hash of the `SourceId` for crates.io is a well-known
+// value.
+//
+// Note that the hash value matches what the crates.io source id has hashed
+// since long before Rust 1.30. We strive to keep this value the same across
+// versions of Cargo because changing it means that users will need to
+// redownload the index and all crates they use when using a new Cargo version.
+//
+// This isn't to say that this hash can *never* change, only that when changing
+// this it should be explicitly done. If this hash changes accidentally and
+// you're able to restore the hash to its original value, please do so!
+// Otherwise please just leave a comment in your PR as to why the hash value is
+// changing and why the old value can't be easily preserved.
+#[test]
+fn test_cratesio_hash() {
+    let config = Config::default().unwrap();
+    let crates_io = SourceId::crates_io(&config).unwrap();
+    assert_eq!(crate::util::hex::short_hash(&crates_io), "1ecc6299db9ec823");
+}
+
 /// A `Display`able view into a `SourceId` that will write it as a url
 pub struct SourceIdAsUrl<'a> {
     inner: &'a SourceIdInner,
