Skip to content

complete the migration from thiserror to gix-error - #2847

Draft
Sebastian Thiel (Byron) wants to merge 32 commits into
mainfrom
gix-error-completion
Draft

complete the migration from thiserror to gix-error#2847
Sebastian Thiel (Byron) wants to merge 32 commits into
mainfrom
gix-error-completion

Conversation

@Byron

Copy link
Copy Markdown
Member

Tasks

This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.

  • refackiew

Everything below this line was generated by Codex GPT-5.

Created by Codex on behalf of Byron. Byron will review before this is ready to merge.

Reported issue

$issue-full-auto etc/plan/gix-error.md lays out a plan to replace thiserror with gix-error. For each crate to replace thiserror in, also check if any of its variants is matched on. If so, hand-expand to the code that thiserror would produce and remove it. Otherwise, use gix-error in its place. Each commit should pass cargo check --workspace --all-targets.

Finally, gix (crate) should be able to use gix-error::Error via gix::Error exclusively and mostly use ?. Note that in gix there is also utilities to see if certain errors can be retried - this functionality should be put into gix-error, probably directly on gix-error::Error.

The gitoxide-core and gitoxide crates should keep anyhow, and that should work natively with gix-error - probably gix will have to forward the gix-error/anyhow feature to achieve that as well.

RetryableError should only be used when the error otherwise is too specific. If gix-error can inspect an error chain with well-known errors, it should do that. Keep an eye out for other standard classifications such as ValidationError; NotFound should be a well-known gix-error type. Object-kind mismatch can be a ValidationError.

Assuming all plumbing crates have already been processed so only thiserror in gix is left: avoid hand-expanded pattern-matched enums when gix::Error classification or a source-chain search for well-known plumbing errors works instead.

Refs #2351

Summary

  • removes direct thiserror use from workspace crates and exposes top-level failures through gix::Error
  • adds standard retry, corruption, not-found, and validation classifications to gix-error
  • determines retryability from known source-chain errors, retaining the explicit retry wrapper only at dependency-specific boundaries
  • preserves concrete source chains, classifications, and probable causes when errors are converted and raised again
  • keeps anyhow in the binaries and forwards the gix-error/anyhow feature through gix

Validation

  • cargo check --workspace --all-targets
  • cargo test -p gix-error
  • cargo test -p gix-error --features auto-chain-error --test auto-chain-error
  • cargo test -p gix --test gix revision::spec::
  • remote, clone, credential-helper, shallow-clone, and blocking/async network feature tests during the migration
  • one Codex commit review per final commit hash

Sebastian Thiel (Byron) pushed a commit that referenced this pull request Jul 28, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
@ameyypawar

Copy link
Copy Markdown
Contributor

Did the comparison. Three things, plus one offer.

1. fetch::Error::Negotiate truncates the chain. Its source() returns None, and unlike the Http arm — which iterates the Exn's frames directly — Negotiate falls through to gix_error::can_retry(self), which walks source(). So anything retryable below a negotiate failure is unreachable. It's public-API surface rather than a live bug: nothing in-tree calls fetch::Error::can_retry today, and receive_pack already remaps Negotiate into a CorruptionError. Worth noting client::Error::source() returns None for Http and SshInvocation too — the frame-iterating arm compensates can_retry specifically, but anything else walking source() would hit the same wall.

2. is_not_found() matches any io::ErrorKind::NotFound anywhere in the chain, and it's the discriminator at Submodule::open(), Repository::head() and head_tree_id_or_empty(). I couldn't produce a concrete path where that misfires — unborn heads raise a marked NotFoundError, missing-object errors are unmarked and propagate correctly — but the predicate is broader than the question being asked at each site, and a marker planted deeper later would change behaviour silently. Worth a second look rather than a bug report.

3. from_error on something already a gix_error::Error flattens its chain. Exn::new degenerates every source below the top to strings. It compiles and the suite passes, because the classifiers downcast to crate::Error and recurse. Live instances exist — e.g. self.head().map_err(gix_error::Error::from_error) in gix/src/repository/index.rs, where head() already returns gix_error::Error.

I've written a guard for that: from_error returns the value unchanged when it's already an Error, plus a #[track_caller] debug_assert naming the caller. Silent in release, loud in tests, no unsafe. Happy to send it as its own small PR — it protects any future conversion.

Also: in traversal_names_do_not_escape_the_modules_directory, the three erased-API assertions (git_dir_try_old_form, open, state) went from matching ParentComponent to is_validation(). The first assertion on sm.git_dir() still checks the specific error, so the test isn't toothless — but those three no longer distinguish a traversal rejection from any other validation failure.

On the comparison: I audited every source() arm in #2716 — 541 across 168 hand-written impls — against the derives they replaced. All match. And the guard's assert never fired across cargo test -p gix (416 tests), so no double-wrap on any tested path. That's a runtime check over tested paths, not a static proof.

There's more from the sweep — a per-type verdict on all 42 types #2716 left concrete, an erasure order for the E0119 chains, and a list of dropped #[error] messages. Say the word if any of that is useful.

@Byron

Copy link
Copy Markdown
Member Author

Thanks Amey Pawar (@ameyypawar), while noting that I find no pleasure in reading these AI generated blobs of text.

My main gripe is that it's a bot speaking through you, so unless you say you produced this text by hand or think you could produce it, disclosure is the way to go. I recommend adding a few lines of yourself on top giving me your verdict, no matter what it is (i.e. something like "this looks reasonable to me, and I spot-checked one of these claims"), followed, by a separator to clearly mark the AI blob.

Thanks again.

Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 4, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 4, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 4, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
@ameyypawar

Copy link
Copy Markdown
Contributor

Re-checked after your push. Negotiate and client::Error look right, and the repository/index.rs double-wrap is gone.

Four things I'd still look at. The message loss bothers me most; on the security tests I'd rather have your call than mine.

The sweep and this write-up are both AI-produced — I took help of AI tools throughout. I checked the two source arms and the index.rs wrap myself.


Messages dropped: 53 sites, 50 distinct. Worst: clone/fetch/mod.rs and config/mod.rs+config/tree at 9 each, update_refs/update.rs at 7 (now a bare alias, nothing re-attached), repository/mod.rs at 5.

17 assertions weakened. Two matter: the three erased-API asserts in traversal_names_do_not_escape_the_modules_directory are bare is_validation() (assertion 1 still pins ParentComponent), and remote/connect.rs:14 lost ProtocolDenied { scheme: File }.

is_not_found() matches any raw io NotFound at any depth; is_validation() has no io disjunct. So the looser one guards head_tree_id_or_empty(). Correcting myself from last time — I said I found no misfire, but detached HEAD with a missing object yields the empty tree. Symbolic HEAD is fine.

Dead branches: clone/fetch/mod.rs:253 and update_refs/mod.rs:207 downcast out of err.sources(), which never matches in chain mode. gix defaults to auto-chain-error, binaries build tree mode — so library consumers lose those paths. (gitoxide-core/repository/diff.rs:130 too, but that predates this branch.)

Also: ~31 double-wraps left after the 19 you removed — a floor, counted from monomorphised instantiations rather than grep, so I can pull the list if useful. And 97b7a7cab leaves probable_cause() on the truncated node.

Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 5, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 5, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 5, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
@Byron

Copy link
Copy Markdown
Member Author

Thanks Amey Pawar (@ameyypawar). This is an interesting experiment as you essentially take the role of a reviewer, while my agent double-checks and fixes. And all that without any human review, so I am already very curious on how the actual review can be done efficiently.
Meantime, agents do things through their meat-proxies 😅.

Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 5, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 8, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 18, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
@Byron Sebastian Thiel (Byron) changed the title change!: complete the migration from thiserror to gix-error complete the migration from thiserror to gix-error Aug 18, 2026
Sebastian Thiel (Byron) pushed a commit that referenced this pull request Aug 19, 2026
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
# ee02574 feat: classify corrupt data in gix-error

feat: classify corrupt data in gix-error

# 9a6931865c feat: classify missing resources in gix-error

feat: classify missing resources in gix-error

# 2a5cf4e895 feat: convert boxed errors into gix-error

feat: convert boxed errors into gix-error

# d83cbf1aa6 feat: classify invalid input in gix-error

feat: classify invalid input in gix-error

# ec3bfdfc2a fix: retain classification through nested gix errors

fix: retain classification through nested gix errors

# 54d8a86d2d fix: preserve copied error source chains

fix: preserve copied error source chains

# d2e1310710 fix: preserve probable causes when flattening error trees

fix: preserve probable causes when flattening error trees

# 77138dde03 fix: expose stored error types in chain mode

fix: expose stored error types in chain mode

# 4913a5b66b fix: preserve nested gix errors when raising context

fix: preserve nested gix errors when raising context

# e19fc18432 fix: retain typed probable causes through nested gix errors

fix: retain typed probable causes through nested gix errors
Hand-expand the traversal errors because callers classify cancellation and lookup failures by variant.

Validation: cargo test -p gix-traverse --all-targets --features sha1; cargo check --workspace --all-targets
Hand-expand discovery errors because callers classify repository search and validation outcomes by variant.

Validation: cargo test -p gix-discover --all-targets --features sha1; cargo check --workspace --all-targets
Hand-expand credential errors because helper fallback, missing credentials, and protocol failures are classified by callers.

Validation: cargo test -p gix-credentials --all-targets; cargo check --workspace --all-targets
Hand-expand directory-walk errors because callers classify interruption and root failures by variant.

Validation: cargo test -p gix-dir --all-targets --features sha1; cargo check --workspace --all-targets
Hand-expand status errors because their variants remain part of caller-visible classification.

Validation: cargo test -p gix-status --all-targets --all-features; cargo check --workspace --all-targets
Hand-expand blame errors because callers classify invalid ranges and traversal failures by variant.

Validation: cargo test -p gix-blame --all-targets; cargo check --workspace --all-targets
Hand-expand checkout errors because delayed-filter and path failures remain caller-visible classifications.

Validation: cargo test -p gix-worktree-state --all-targets --features sha1; cargo check --workspace --all-targets
Hand-expand submodule configuration errors because invalid path, URL, update, and pathspec outcomes are matched by callers.

Validation: cargo test -p gix-submodule --all-targets --features sha1; cargo check --workspace --all-targets
# cbe6997 Refresh of migrate-transport-errors

Refresh of migrate-transport-errors

# b956d44 feat!: remove thiserror from gix-ref

feat!: remove thiserror from gix-ref

# b6fcd11 feat!: remove thiserror from gix-pack

feat!: remove thiserror from gix-pack

# 59cbc94 feat!: remove thiserror from gix-odb

feat!: remove thiserror from gix-odb

# d145f21 fix: gate HTTP retry inspection with its feature

fix: gate HTTP retry inspection with its feature
# 1436ecb feat!: migrate object-facing gix errors to gix::Error

feat!: migrate object-facing gix errors to gix::Error

# 83ede2b feat!: expose repository open failures as gix::Error

feat!: expose repository open failures as gix::Error

# bd47992 feat!: expose repository initialization failures as gix::Error

feat!: expose repository initialization failures as gix::Error

# 217dcc2 feat: classify invalid input in gix-error

feat: classify invalid input in gix-error

# b8c9fb4 feat!: expose gix configuration failures as gix::Error

feat!: expose gix configuration failures as gix::Error

# 508c1b1 feat!: expose repository failures as gix::Error

feat!: expose repository failures as gix::Error

# ccd246f feat!: expose reference failures as gix::Error

feat!: expose reference failures as gix::Error

# 2b9ed88 feat!: expose submodule failures as gix::Error

feat!: expose submodule failures as gix::Error

# 5ece2a2 feat!: expose worktree and status failures as gix::Error

feat!: expose worktree and status failures as gix::Error

# 8352acd migrate remaining gix errors to gix-error

migrate remaining gix errors to gix-error
CI exposed three remaining migration adaptations: gix-shallow doctests could not convert Exn through Box<dyn Error>, the ein init journey snapshots still expected the removed enum message, and lint rejected two mechanical expressions. Convert Exn explicitly in the doctest, update both init snapshots, and apply the two lint-preserving rewrites.

Observed in test-doc, test-journey, and lint on PR #2847.
Preserved source chains now retain the handshake context above SSH argument validation failures. Update every receive and clone journey snapshot in this family together.
CI uses the current stable formatter and requires six mechanical layout updates in migrated files.
Preserve the public-facing context from the former thiserror variant while retaining the interpolation error as its source.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants