Restore crates.io's SourceId hash value to before

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,