Convert the plumbing crates away from thiserror#2716
Convert the plumbing crates away from thiserror#2716Amey Pawar (ameyypawar) wants to merge 54 commits into
Conversation
![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: fb20cf1226
ℹ️ 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".
| impl std::error::Error for Error { | ||
| fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { | ||
| match self { | ||
| Error::Io(err) => err.source(), |
There was a problem hiding this comment.
When bytes_of_file() or bytes_with_hasher() returns Error::Io for an open/read/interruption failure, this now exposes err.source() instead of the wrapped std::io::Error itself. For ordinary I/O errors source() is usually None (and for Error::other it skips the io::Error layer), whereas the previous #[error(transparent)] variant made the I/O error available as the source; callers using error chains/downcasting lose the actual I/O error and its kind. Return Some(err) for this variant.
Useful? React with 👍 / 👎.
|
Thanks a lot! When reading the review notes, which I find very valuable, I'd hope that these places to watch out for are marked in code in some way. Maybe Besides that, and if you think the agent does a good job, I think this can continue to spike the whole task, the complete removal of Lastly, something to think about, is to see which other standard-errors should exist as part of Thanks again for all your work! |
fb20cf1 to
15b684b
Compare
|
Done on all counts:
I'll continue with the remaining batches toward complete Also: the earlier |
|
Fantastic, thank you! Please do feel free to put the review markers into their respective commits, and to rewrite them at will. Some future work might want you to change bigger bits, and rewriting is fine with me. I will let you know once I start working on this PR so we don't step on each other feet. One more note: If you think that converting multiple related crates at once would be beneficial, please feel free to do so. My plan is to look at the whole PR at some point, and try to find my way from there. |
|
It would also be helpful if the merge-commit could be removed for a clean patch-stack. Besides that, I would have hoped for more lines to be removed, but maybe that changes once |
|
The final result will land as a clean linear patch-stack (one commit per crate, no merge commits) — I'll flatten it as the last step before it leaves draft. |
Adapt gitoxide-core, the CLI, gix tests and gix-testtools to the converted crates' error types; drop the removed thiserror entries from the lockfile and bring the migration plan up to date.
0e60087 to
af178a1
Compare
|
The branch is now the clean patch-stack: 29 Deliberately not in this PR:
Converted files still carry the allow↔expect lint-suppression drift from your lint pass (left for one sweep rather than touching every crate); the substantive bits my stale branches had reverted (gix-merge's binary-pick refactor, gix-odb's PartialOrd + allowances) are restored. CI stays red until |
Amey Pawar (ameyypawar)
changed the title
|
For this to really come to fruition, I think it will need All other plumbing crates that weren't converted should also be converted, no need to avoid conflicts. Conflicts are easy to fix nowadays. Thanks again! |
The single `to_normal_path_components::Error` enum becomes `Exn<ValidationError>` per the plan's validation-path rule — no consumer names or matches its variants, and both in-crate io boundaries convert via Display or `.into_error()`. Both failure cases now carry the offending input (path/component bytes) for introspectability; the input is rendered with debug quoting after the message where the path was previously interpolated inline. The crate-level doc example adapts with `.into_error()` as `Exn` deliberately does not implement `std::error::Error`.
The offending input is now rendered debug-quoted after the message instead of interpolated inline, per the ValidationError conversion.
The remaining message assertion adapts to the debug-quoted input rendering; rustfmt reflows the multi-line construction; the lockfile picks up the thiserror->gix-error dependency swap.
…r on Windows gix_path::into_bstr expects convertible paths and panics on ill-formed UTF-16; the previous thiserror variant construction was infallible. Use try_into_bstr with a lossy fallback so error construction can never abort.
All eight error enums (command validation, ls-refs, handshake + ref parsing, fetch: error/negotiate/refmap/response) become hand-written Display/Error/From impls — concrete types are load-bearing here: the four IsSpuriousError impls match variants, and the enums cross-embed each other and are consumed by gix and gitoxide-core. Display texts are preserved verbatim; transparent variants forward Display and source(); the shallow/lock variants wrap Exn types (which deliberately do not implement std::error::Error) and expose the inner error via &**err as their source(); boxed and #[source]-only fields keep their no-From semantics. The test-local fetch error converts the same way, letting the dev-dependency on thiserror go as well. Verified with the blocking-client and async-client feature combos (57 tests each), crate-scoped clippy, and fmt.
Re-convert the crate on top of the lifetime-free parser refactor: all 13 error types (the generic lookup Error<E> with the bounds thiserror would have inferred, the span error struct, the macro-generated name/value_name errors, section header/value, file rename/set_raw_value, includes, and the four init errors) become hand-written Display/Error/From impls with texts preserved verbatim. The includes Realpath variant wraps gix-path's Exn and exposes the inner error via &**err as its source(). The gitdir test utility adapts with .into_error() where it propagates the realpath Exn into a std-Error result. Verified with the full crate suite (389 tests incl. doctests), crate-scoped clippy, and fmt.
The gitdir test utility names gix_error::Exn::into_error per clippy's redundant-closure suggestion, which needs gix-error as a dev-dependency.
All seven error types become hand-written Display/Error/From impls with texts preserved verbatim: the five field-query errors in config.rs (text structs and enums whose `source`-named fields surface via source() without gaining a From), the init error whose three transparent variants forward to the freshly converted gix-config types, and the is-active-platform error whose two variants wrap gix-pathspec `Exn` types (which deliberately do not implement std::error::Error) and expose the inner error via &**err as their source().
Completes the previous commit: the five field-query errors in config.rs and the init error in lib.rs become hand-written Display/Error/From impls with texts preserved verbatim. Fields named `source` surface via source() without gaining a From, exactly like the thiserror-generated code did.
The last of the crate's seven error types: the init error's three transparent variants forward Display and source() to the now-converted concrete gix-config types and keep their From impls.
gix-index's write error was converted against the pre-conversion concrete gix-lock error and returned Some(err) where gix_lock::acquire::Error is now Exn<Failure> (not a std Error) — expose the inner Failure via &**err instead, un-breaking gix-index and everything above it. Also reflow gix-submodule's Display arms per rustfmt and extend the gix-config includes note to disclose that the Realpath variant's source() repeats the message a std chain-walker already printed (the io cause remains reachable in the Exn tree).
…alpath chain shape The gix-config includes note now also states that the Realpath variant's source() yields the same message a std chain-walker just printed — the io cause stays reachable through the Exn frame tree and at erased boundaries.
First module of the `gix` finale, following Byron's direction that `gix` should "just use `gix::Error` for everything": the four-variant `thiserror` enum collapses to `pub type Error = gix_error::Error`, and its construction moves to the single consumer's call sites. Each site keeps the variant's context message via `.and_raise(gix_error::message(...))` (the transparent path-interpolate site uses `Error::from_error`), so introspectability is preserved while the hand-rolled enum, its `Display`/`Error`/`From` impls, and the `thiserror` derive all go away.
Second module of the `gix` finale: the four transparent-variant `thiserror` enum collapses to `pub type Error = gix_error::Error`. All variants forwarded Display/source to their inner error, so the consumer's call sites in `repository/object.rs` convert each with `Error::from_error` (the tag() adaptations land in a separate commit as object.rs is edited directly).
Its three error sites (signature time parse, write_object, tag_reference) now convert their concrete callee errors with gix_error::Error::from_error, matching tag::Error becoming gix_error::Error.
Third module of the finale, and it clears the pre-existing E0119 that blocked the crate: the enum had `NormalizeSpec(#[from] gix_pathspec::normalize::Error)` and `RepoPrefix(#[from] gix_path::realpath::Error)`, both now `Exn<Message>`, so `thiserror` generated `From<Exn<Message>>` twice. Collapsing to `pub type Error = gix_error::Error` removes the duplicate impls. In `Pathspec::new`, the already-`Exn` callees (`parse`, `repo.prefix`, `realpath_opts`) propagate with a plain `?`; `from_specs` keeps the old `NormalizeSpec` message via `.or_raise`; the concrete defaults lookup and the boxed `make_attributes` use `Error::from_error`. The `#[from]` embedders (dirwalk, status, worktree) resolve through the type alias with no edit.
… construction pathspec: `make_attributes` yields `Box<dyn Error>` which doesn't implement `std::error::Error`, so bridge via `io::Error::other` (preserves Display + causes) pending a possible `gix_error::Error::from_boxed`. worktree: the erased `pathspec::init::Error` no longer has a `Defaults` variant, so construct the `gix_error::Error` from the same `pathspec_defaults_ignore_case::Error`.
Fourth module of the finale: the three-variant enum collapses to `pub type Error = gix_error::Error`. In `shorten()`, the two concrete odb callees (`packed_object_count`, `disambiguate_prefix`) that were transparent variants convert with `Error::from_error`, and the `NotFound` message becomes a source-less `Error::from_error(message(...))` preserving the exact text. The `#[from]` embedder in `commit.rs` and the `short_id()` return in `object/commit.rs` resolve through the type alias with no edit.
`gix_error::message()` takes a `&'static str`; formatted messages go through the `message!` macro (which runs `format!` into `Message::new`).
Fifth module of the finale: all four `thiserror` enums (`pipeline::options`, `convert_to_git`, `convert_to_worktree`, `worktree_file_to_object`) collapse to `pub type Error = gix_error::Error`, and their construction moves to the call sites. Transparent variants use `Error::from_error`; the message-bearing ones keep their text verbatim while preserving the cause — `.or_raise` for the attribute-priming reads, and `and_raise` for the IO and `Driver` sites whose named `source` fields previously surfaced via `source()`. `MissingWorktree` has no cause and becomes a source-less message. Errors from the now-erased sibling modules (`options`, `convert_to_git`) propagate with a plain `?`.
…ix::Error` Sixth module of the finale. `options::init::Error` and `new_rewrites::Error` collapse to `pub type Error = gix_error::Error`; the two concrete config lookups in `new_rewrites_inner` convert with `Error::from_error`, and `from_configuration` needs no change since both sides of its `?` are now the same erased type. `resource_cache::Error` is deliberately left as an enum for now: it is embedded alongside the already-erased `pathspec::init::Error` in `status::index_worktree::Error`, so erasing it would produce two `From<gix_error::Error>` impls on that enum. It is therefore converted together with the status cluster, which also has to resolve the generic `From<crate::pathspec::init::Error>` bound in `index_worktree.rs`.
2 participants
Originally the batch-1 PR; scope grew to the whole plumbing conversion as discussed — one
feat!commit per crate (29 crates, leaf-first,gix-lockat the bottom), plus a trailing chore commit for the lockfile, migration plan, and adaptations ingitoxide-core, the CLI,gix's tests andgix-testtools. No merge commits, for cherry-picking.Not in this PR (also noted in
etc/plan/gix-error.md):gix-config,gix-submodule— kept atmain; their conversions predate the lifetime-free config refactor and are best redone on top of it.gix-protocol— waiting on feat: filters and partial cloning: initial support #2375.gix-fs— no conversion yet.gix— the finale; planned as a follow-up PR against current main (where?-out-of-everything lands). Its code is untouched here — only its tests are adapted to the converted crates. Because of that, CI stays red on this PR: the workspace only compiles fully oncegixconverts.Shapes, and when each was used
Exn<Message>/Exn<ValidationError>per the plan's default wherever nothing discriminates variants programmatically; validation/parsing paths carry the offending input viaValidationError.Exn<concrete>(e.g.gix-lock'sFailure) where callers discriminate cases but the gix-error benefits are wanted.Display/Error/Fromimpls (message texts unchanged) where the type is load-bearing — consumer#[from]/#[source]embeds, variant discrimination,Eq/PartialEqin tests,From<E>trait bounds. Mirrors the manual-error entries in Reorganise Error-Handling aroundExn#2351; dropsthiserrorwithout replacement in those crates.Review notes
Exnaliases no longer implementstd::error::Error(by design); in-tree consumers keep working throughDeref, propagation intoBox<dyn Error>/anyhowuses.into_error().TODO(review)markers sit at the known shortcomings and hand-preserved subtleties throughout — the two worth reading first: the io-cause gap through still-unconverted wrappers (gix-path/src/realpath.rs) and the hand-preserved transparent-Io semantics (gix-hash/src/io.rs).gix-attributes: validation errors now render their input with debug quoting (: "foo"), and unquoting failures gain the line number with the cause chained (previously a static message) — that's what the larger test rewrite in its commit reflects.Exn-converted types now assert messages, per the plan; tests of concrete types kept their matches.mainin converted files: the allow↔expect lint-suppression drift from the lint pass (left for one sweep); the substantive changes stale conversion branches had reverted (gix-merge's binary-pick refactor, gix-odb's canonicalPartialOrd+ dropped allowances) are restored.Validated per crate during conversion (crate suites + heaviest consumers, clippy's four feature combos with warnings denied, fmt). The stack is ordered so each crate lands after its converted dependencies; full workspace green arrives with the
gixfollow-up.