feat!: gix-error instead of thiserror in gix-lock#2711
feat!: gix-error instead of thiserror in gix-lock#2711Amey Pawar (ameyypawar) wants to merge 3 commits into
gix-error instead of thiserror in gix-lock#2711Conversation
Replace the thiserror-derived `acquire::Error` enum with `gix_error::Exn<acquire::Failure>`, where `Failure` is a concrete error that keeps actual lock contention distinguishable from other IO errors, as callers rely on telling these apart. Error message texts are unchanged.
Consumers that embed the acquisition error in their own `thiserror` enums keep working unchanged thanks to `Exn`s `Deref` to the concrete `Failure`, which keeps `source()` chains intact. Only variant payloads that need `std::error::Error` themselves now hold `Failure` directly, and `?`-conversions in tests and doctests convert via `into_error()`.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a966aac3a9
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "Codex (@codex) review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "Codex (@codex) address that feedback".
| } | ||
|
|
||
| /// The error returned when acquiring a [`File`] or [`Marker`]. | ||
| pub type Error = gix_error::Exn<Failure>; |
There was a problem hiding this comment.
With gix_lock::acquire::Error now aliased to gix_error::Exn<Failure>, the rest of this commit still has workspace callers that treat it as a normal std::error::Error/From source; I checked the f26 tree with git grep and found examples such as gix-index/src/file/write.rs:10 and gix-protocol/src/fetch/error.rs:21 using #[from] gix_lock::acquire::Error, plus gitoxide-core/src/pack/multi_index.rs:27 using ? into anyhow::Result. Since Exn deliberately does not implement std::error::Error, these crates can no longer build until those call sites are converted like the updated gix-ref paths or this public boundary keeps an error type that implements Error.
Useful? React with 👍 / 👎.
|
Small update: while waiting on this one I kept going leaf-first and have the rest of batch 1 converted and validated locally — gix-path, gix-attributes, gix-url, gix-zlib, gix-packetline and gix-hash, one crate per branch, each with the affected test suites and the full clippy/fmt matrix green. Would you prefer them as sequential PRs after this review, or as the one overview-PR with one crate per commit that you mentioned, for cherry-picking? Happy either way. |
|
That's exciting! And I am already afraid of the review, but it must be done :D. Let's keep this PR as is, but you can create a new one with all the additional work you have done. That way I can take a look as well. |
|
Amey Pawar (@ameyypawar) - let's move over to the big PR, and put the commit here presumably at the bottom. |
|
Moved into #2716 as the bottom commits, as requested — closing in favor of the one PR. |
2 participants
The first crate of the
gix-errorconversion we discussed — the smallest pending leaf frometc/plan/gix-error.md, kept deliberately small to calibrate on your review before ramping up.The shape
The plan prefers
pub type Error = Exn<Message>unless a crate needs a more specific concrete error — this one does:gix-refdeliberately distinguishes lock contention from other IO errors when preparing ref transactions (itslock_acquire_error()documents that path collisions must surface asError::Io, whileLockAcquireis "reserved for actual contention"). So the thiserror enum became a concreteacquire::Failurewith manualDisplay/Errorimpls — message texts unchanged — wrapped aspub type Error = Exn<Failure>, and that matcher now reaches it throughinto_inner().What made this smaller than expected
Consumers that merely embed the error in their own
thiserrorenums via#[from](gix-index,gix-protocol) need no changes at all:Exn'sDereflets the derivedsource()resolve to the concreteFailure, sostderror chains keep exactly the same links as before — verified with a probe walking a consumer's chain down to the underlyingio::Error. That should shrink most future conversions, as only consumers that match on variants need adapting.Adaptations (second commit)
gix-ref: the matcher above, and the two variants holding the failure as#[source]now storeacquire::Failuredirectly. Note this changes the payload type of two publicErrorvariants — happy to split that into its ownchange!:commit if you prefer it called out per-crate.gix-lock/gix-shallowand one?-conversion ingix-testtoolsuse.into_error()perAGENTS.md. Worth knowing:cargo nextestdoesn't execute doctests, so these only show up incargo test --doc.Validated with the test suites of
gix-lock,gix-ref,gix-index,gix-shallow,gix-protocolandgitoxide, plus clippy and fmt.Cargo.lockis included sincepure-rust-buildruns with--locked.