Previously, the `RepoId` was backed by the `git2::Oid` type, which has a `Hash`
implementation that uses `std:#️⃣:Hasher::hash`. This uses a length prefixed
form for hashing.
When switching to `radicle_oid::Oid` and implementing `Hash` without the length
prefix, this subtly changed the hashes going forward.
These hashes are used in `BloomFilter`, which is the type underlying `Filter`.
This resulted in cases where a previous `Filter` that certainly contained
`rad:z3gqcJUoA1n9HaHKufZs5FCSGazv5` were suddenly reporting that they did not
contain this repository.
Changing the implementation of `Hash` for `radicle_oid::Oid` fixes this, and is
confirmed by adding the regression test in `filter.rs`.
I was greeted by `rad patch redact` with
called `Result::unwrap()` on an `Err` value:
Error { code: None, message: Some("failed to convert") }
stack backtrace:
[…]
3: core::result::Result<T,E>::unwrap
at …/rust-1.88.0/lib/rustlib/src/rust/library/core/src/result.rs:1137:23
4: sqlite::cursor::Row::read
at …/index.crates.io-1949cf8c6b5b557f/sqlite-0.32.0/src/cursor.rs:136:9
5: radicle::cob::patch::cache::query::find_by_revision
at ./crates/radicle/src/cob/patch/cache.rs:624:65
6: <… as radicle::cob::patch::cache::Patches>::find_by_revision
at ./crates/radicle/src/cob/patch/cache.rs:553:9
7: radicle_cli::commands::rad_patch::redact::run
at ./crates/radicle-cli/src/commands/patch/redact.rs:23:9
8: radicle_cli::commands::rad_patch::run
at ./crates/radicle-cli/src/commands/patch.rs:1026:13
[…]
It turns out that `sqlite::cursor::Row::read` is the panicky version of
`sqlite::cursor::Row::try_read` (which returns a `Result`).
While it is somewhat rare that SQLite reads fail, it is not unheard of.
In `radicle-cli` it might not be critical, but also `radicle-protocol`
and `radicle-fetch` are affected, and they could potentially panic a
`radicle-node` process.
Use `try_read` instead, and propagate down the error handling.
Remove `fn deserialize` and instead provide a trait `fn decode_exact`
for testing.
Refactor errors, so that we can now cleanly distinguish between
`Error::UnexpectedEnd` which means that decoding might work given
more data, and `Error::Invalid`, which means that no amount of
additional data will decode.
While at it, cleanup the variants of `Error::Invalid`, and provide `ControlType`.
What made the issue fixed in the parent commit harder to spot
for me was the multitude of concepts and terms involved in
encoding: What's *serializing* vs. *encoding*? Why is there
this odd `Frame::to_bytes`?
So I decided to clean up a bit. I removed `fn serialize` so that the
logic stays close to the data, and instead of it provide
`fn encode_vec` in `trait Encode` so that we don't add up with
one-offs like `Frame::to_bytes` again.
Also we make use of `encode_vec` in the tests.
Mostly changes of `use`s and moving components to `radicle-protocol`,
with the goal of just getting `radicle-node` to work on top of the new
`radicle-protocol` crate.
Yaroslav Halchenko
Fintan Halpenny
Lorenz Leutgeb