shorten comment on Ord for SourceKind (#15029)

As discussed in today's cargo meeting, and in
https://rust-lang.zulipchat.com/#narrow/channel/246057-t-cargo/topic/while.20we're.20breaking.20stable.20hash,
we are keeping the manual impl but shortening the comment about why it
is needed.

Author: Eh2406

Cargo.lock

@@ -1,6 +1,6 @@
 [package]
 name = "cargo-util-schemas"
-version = "0.7.2"
+version = "0.7.3"
 rust-version = "1.83"  # MSRV:1
 edition.workspace = true
 license.workspace = true

crates/cargo-util-schemas/Cargo.toml

@@ -1,7 +1,7 @@
 use std::cmp::Ordering;
 
 /// The possible kinds of code source.
-#[derive(Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, PartialEq, Eq)]
 pub enum SourceKind {
     /// A git repository.
     Git(GitReference),
@@ -17,6 +17,19 @@ pub enum SourceKind {
     Directory,
 }
 
+// The hash here is important for what folder packages get downloaded into.
+// Changes trigger all users to download another copy of their crates.
+// So the `stable_hash` test checks that we only change it intentionally.
+// We implement hash manually to callout the stability impact.
+impl std::hash::Hash for SourceKind {
+    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
+        core::mem::discriminant(self).hash(state);
+        if let SourceKind::Git(git) = self {
+            git.hash(state);
+        }
+    }
+}
+
 impl SourceKind {
     pub fn protocol(&self) -> Option<&str> {
         match self {
@@ -31,59 +44,9 @@ impl SourceKind {
     }
 }
 
-/// 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 weird 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.
+// The ordering here is important for how packages are serialized into lock files.
+// We implement it manually to callout the stability guarantee.
+// See https://github.com/rust-lang/cargo/pull/9397 for the history.
 impl Ord for SourceKind {
     fn cmp(&self, other: &SourceKind) -> Ordering {
         match (self, other) {

crates/cargo-util-schemas/src/core/source_kind.rs

@@ -762,7 +762,7 @@ mod tests {
     // 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
+    // since Rust 1.84.0. 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.
     //
@@ -771,6 +771,11 @@ mod tests {
     // 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.
+    // If it takes an ugly hack to restore it,
+    // then leave a link here so we can remove the hack next time we change the hash.
+    //
+    // Hacks to remove next time the hash changes:
+    // - (fill in your code here)
     //
     // The hash value should be stable across platforms, and doesn't depend on
     // endianness and bit-width. One caveat is that absolute paths on Windows
@@ -782,6 +787,11 @@ mod tests {
         use std::hash::Hasher;
         use std::path::Path;
 
+        use snapbox::assert_data_eq;
+        use snapbox::str;
+        use snapbox::IntoData as _;
+
+        use crate::util::hex::short_hash;
         use crate::util::StableHasher;
 
         #[cfg(not(windows))]
@@ -792,68 +802,68 @@ mod tests {
         let gen_hash = |source_id: SourceId| {
             let mut hasher = StableHasher::new();
             source_id.stable_hash(ws_root, &mut hasher);
-            Hasher::finish(&hasher)
+            Hasher::finish(&hasher).to_string()
         };
 
         let source_id = SourceId::crates_io(&GlobalContext::default().unwrap()).unwrap();
-        assert_eq!(gen_hash(source_id), 7062945687441624357);
-        assert_eq!(crate::util::hex::short_hash(&source_id), "25cdd57fae9f0462");
+        assert_data_eq!(gen_hash(source_id), str!["7062945687441624357"].raw());
+        assert_data_eq!(short_hash(&source_id), str!["25cdd57fae9f0462"].raw());
 
         let url = "https://my-crates.io".into_url().unwrap();
         let source_id = SourceId::for_registry(&url).unwrap();
-        assert_eq!(gen_hash(source_id), 8310250053664888498);
-        assert_eq!(crate::util::hex::short_hash(&source_id), "b2d65deb64f05373");
+        assert_data_eq!(gen_hash(source_id), str!["8310250053664888498"].raw());
+        assert_data_eq!(short_hash(&source_id), str!["b2d65deb64f05373"].raw());
 
         let url = "https://your-crates.io".into_url().unwrap();
         let source_id = SourceId::for_alt_registry(&url, "alt").unwrap();
-        assert_eq!(gen_hash(source_id), 14149534903000258933);
-        assert_eq!(crate::util::hex::short_hash(&source_id), "755952de063f5dc4");
+        assert_data_eq!(gen_hash(source_id), str!["14149534903000258933"].raw());
+        assert_data_eq!(short_hash(&source_id), str!["755952de063f5dc4"].raw());
 
         let url = "sparse+https://my-crates.io".into_url().unwrap();
         let source_id = SourceId::for_registry(&url).unwrap();
-        assert_eq!(gen_hash(source_id), 16249512552851930162);
-        assert_eq!(crate::util::hex::short_hash(&source_id), "327cfdbd92dd81e1");
+        assert_data_eq!(gen_hash(source_id), str!["16249512552851930162"].raw());
+        assert_data_eq!(short_hash(&source_id), str!["327cfdbd92dd81e1"].raw());
 
         let url = "sparse+https://your-crates.io".into_url().unwrap();
         let source_id = SourceId::for_alt_registry(&url, "alt").unwrap();
-        assert_eq!(gen_hash(source_id), 6156697384053352292);
-        assert_eq!(crate::util::hex::short_hash(&source_id), "64a713b6a6fb7055");
+        assert_data_eq!(gen_hash(source_id), str!["6156697384053352292"].raw());
+        assert_data_eq!(short_hash(&source_id), str!["64a713b6a6fb7055"].raw());
 
         let url = "file:///tmp/ws/crate".into_url().unwrap();
         let source_id = SourceId::for_git(&url, GitReference::DefaultBranch).unwrap();
-        assert_eq!(gen_hash(source_id), 473480029881867801);
-        assert_eq!(crate::util::hex::short_hash(&source_id), "199e591d94239206");
+        assert_data_eq!(gen_hash(source_id), str!["473480029881867801"].raw());
+        assert_data_eq!(short_hash(&source_id), str!["199e591d94239206"].raw());
 
         let path = &ws_root.join("crate");
         let source_id = SourceId::for_local_registry(path).unwrap();
         #[cfg(not(windows))]
         {
-            assert_eq!(gen_hash(source_id), 11515846423845066584);
-            assert_eq!(crate::util::hex::short_hash(&source_id), "58d73c154f81d09f");
+            assert_data_eq!(gen_hash(source_id), str!["11515846423845066584"].raw());
+            assert_data_eq!(short_hash(&source_id), str!["58d73c154f81d09f"].raw());
         }
         #[cfg(windows)]
         {
-            assert_eq!(gen_hash(source_id), 6146331155906064276);
-            assert_eq!(crate::util::hex::short_hash(&source_id), "946fb2239f274c55");
+            assert_data_eq!(gen_hash(source_id), str!["6146331155906064276"].raw());
+            assert_data_eq!(short_hash(&source_id), str!["946fb2239f274c55"].raw());
         }
 
         let source_id = SourceId::for_path(path).unwrap();
-        assert_eq!(gen_hash(source_id), 215644081443634269);
+        assert_data_eq!(gen_hash(source_id), str!["215644081443634269"].raw());
         #[cfg(not(windows))]
-        assert_eq!(crate::util::hex::short_hash(&source_id), "64bace89c92b101f");
+        assert_data_eq!(short_hash(&source_id), str!["64bace89c92b101f"].raw());
         #[cfg(windows)]
-        assert_eq!(crate::util::hex::short_hash(&source_id), "01e1e6c391813fb6");
+        assert_data_eq!(short_hash(&source_id), str!["01e1e6c391813fb6"].raw());
 
         let source_id = SourceId::for_directory(path).unwrap();
         #[cfg(not(windows))]
         {
-            assert_eq!(gen_hash(source_id), 6127590343904940368);
-            assert_eq!(crate::util::hex::short_hash(&source_id), "505191d1f3920955");
+            assert_data_eq!(gen_hash(source_id), str!["6127590343904940368"].raw());
+            assert_data_eq!(short_hash(&source_id), str!["505191d1f3920955"].raw());
         }
         #[cfg(windows)]
         {
-            assert_eq!(gen_hash(source_id), 10423446877655960172);
-            assert_eq!(crate::util::hex::short_hash(&source_id), "6c8ad69db585a790");
+            assert_data_eq!(gen_hash(source_id), str!["10423446877655960172"].raw());
+            assert_data_eq!(short_hash(&source_id), str!["6c8ad69db585a790"].raw());
         }
     }