ex_TABmVr_CTEY6B5C-NDKk7ciKdCK1bLf6ltjpXyoVknvyAC2AnRjA5rlPX_5ORQMeHmzJ2UxWUR0RdgBjYAQ
----- END CREV PROOF -----
+----- BEGIN CREV PROOF -----
+kind: package review
+version: -1
+date: "2022-01-14T02:22:52.039384447+11:00"
+from:
+ id-type: crev
+ id: QE5OVlHZ4QyOcMqdjXhS1MgsoZHvUqxOHNZwyfpsDIU
+ url: "https://git.chrismorgan.info/crev-proofs"
+package:
+ source: "https://crates.io"
+ name: same-file
+ version: 1.0.6
+ revision: 5799cd323b8eefd17a089c950dac113f66c89c9e
+ digest: Yf7qyYw3W3WoHK4t74epRoJkZehfhWh-Hh-R0LfB8UQ
+review:
+ thoroughness: high
+ understanding: high
+ rating: neutral
+comment: |-
+ The good:
+
+ ✓ Decently tested, including CI coverage of the declared MSRV (1.34.0). (As
+ noted in src/lib.rs lines 485–491, edge case tests are difficult to
+ synthesise; Andrew doesn’t know how to create those environmental
+ conditions and I don’t really either. I suppose it’d entail reading file
+ system specs and creating suitable file systems from scratch, and mounting
+ them—not terribly difficult, but tedious work for limited gain.)
+
+ ✓ The crate only supports its *functionality* on cfg(any(unix, windows)), but
+ still provides a sane fallback on cfg(not(any(unix, windows))) so that all
+ methods returns an explanatory I/O error.
+
+ ✓ It’s unfortunate that false positives can arise, but given that it’s
+ unavoidable, it is pleasingly well-document.
+
+ ✓ Cargo.toml excludes CI files from the package (keep things small!)
+
+ The neutral:
+
+ • The I/O error returned on unknown platforms is of kind Other. I look
+ forward to an MSRV bump to 1.53.0 so that Unsupported can be used.
+
+ • The code has some cfg(target_os = "redox") that can be removed when the
+ MSRV reaches Rust 1.38.0 (that’s when redox was moved to the unix family,
+ in <https://github.com/rust-lang/rust/pull/60547>).
+
+ • Andrew uses his preferred Unlicense/MIT combo; [I have problems with
+ Unlicense <https://chrismorgan.info/blog/unlicense/>].
+
+ • src/win.rs lines 17–32 acknowledge a straightforwardly-remediable gap that,
+ if resolved, would by the sound of it eliminate false positives on at least
+ the Windows Server + ReFS combo. This is something that someone that cared
+ could contribute fairly easily, with access to Windows Server 2012 or
+ later. (Microsoft’s documentation of FILE_ID_INFO *says* it’s Server-only,
+ but given that ReFS is supported from Windows 8.1 onwards as well, I wonder
+ if it might be *de facto* available elsewhere too.)
+
+ • Handle::{stdin, stdout, stderr} bypass std::io::{Stdin, Stdout, Stderr}
+ locking. I don’t much like this, but given that avoiding it would be very
+ painful to the API, and that locking isn’t a memory-safety matter, already
+ not being inviolable due to foreign code (though the std::io docs don’t
+ seem to mention that), so I think this was the pragmatic choice.
+
+ The bad:
+
+ ✗ The file size comparison that is documented in two places as being there on
+ Windows as a false positive avoidance heuristic is accidentally missing.
+ This was reported and acknowledged as an error on 2021-05-19 in
+ <https://github.com/BurntSushi/same-file/issues/52>, but it still hasn’t
+ been reinstated or the comments removed.
+
+ ✗ Handle::{dev, ino} are Unix-specific methods; it would be better not to
+ expose them on the type itself, but to move them to a platform-specific
+ extension trait to make it more explicit, e.g. HandleUnixExt.
+
+ ✗ std::os::unix::io::IntoRawFd implementation panics on Handle::{stdin,
+ stdout, stderr}, due to careless Option handling (and the unwraps are
+ accompanied by comments assuring you that it won’t panic 🙁).
+ ManuallyDrop would be a better choice than Option here. Demonstration:
+ ``std::os::unix::io::IntoRawFd::into_raw_fd(same_file::Handle::stdin().unwrap())``.
+ Reported at https://github.com/BurntSushi/same-file/issues/55.
+
+ I was going to rate it positive until I found this panic (the first two bads
+ don’t warrant a downgrade). Fix just that, and I’ll update it to positive.
+----- SIGN CREV PROOF -----
+GlyWB-6rwNvY0qOppjx4AcRh1ta-btcpZ6OBaqPUg_Nr5WGCYWiNsfNWYENFHDLZpnPViTt7YaW78oPepAHhCA
+----- END CREV PROOF -----
+