gix-error punch-through

[Byron]

Tasks

• exn for gix-date
• is it possible to downcast to a particular trait that is implemented?
  • No, as Frame can only cast to specific types, not to anything that one of them implements.
  • However, with Tracking Issue for error_generic_member_access rust-lang/rust#99301 soon being merged, I think it's not problem to one day provide information through that mechanism.
  • Meanwhile, with specific types that are well-known, we can already handle everything that IsSpurious handles today.
• vendor exn for compatibility
  • Error -> ErrorExt
  • Error is top-level type-erased error
  • Utilities to search for types, with filters (i.e. leaf only)
• gix-revision
• make cargo run --bin gix -- diff file CODE_OF_CONDUCT.md CHANGELOG.md work using Error::iter_frames()

Follow-up

• exn for gix-transport

[Byron]

@copilot Remove thiserror from gix-transport and use gix-error instead. Create one crate level error at gix_transport::Error with just the kinds of simple errors that one would want to inspect to understand if connections are retry-able. Be sure to read everything in https://github.com/fast/exn/tree/main/examples/src to learn how to use gix-error, which is derived from exn.

[Copilot]

@Byron I've opened a new pull request, #2360, to work on those changes. Once the pull request is ready, I'll request review from you.

[Byron]

@copilot Remove thiserror from gix-revision and use gix-error instead. Create one crate level error at gix_transport::Error with just the kinds of simple errors that one would want to inspect to understand if connections are retry-able. Be sure to read everything in https://github.com/fast/exn/tree/main/examples/src to learn how to use gix-error, which is derived from exn.

[Copilot]

@Byron I've opened a new pull request, #2361, to work on those changes. Once the pull request is ready, I'll request review from you.

[Byron]

@copilot Add anyhow as optional feature to gix-error in a new PR and make it so that both Exn<T> and Error in gix-error can be converted to anyhow::Error in such a way that the tree of error frames is flattened into source() relationships. Validate that an anyhow::Error created like this will print as error chain.

[Copilot]

@Byron I've opened a new pull request, #2371, to work on those changes. Once the pull request is ready, I'll request review from you.

[Copilot]

Pull request overview

This PR introduces a new error handling system for gitoxide by adding the gix-error crate. The goal is to provide better error context tracking with call-site information and support for error trees (multiple error causes), moving away from thiserror-based errors to a custom solution inspired by exn.

Changes:

• Adds new gix-error crate with Error, Exn<E>, Message, and ParseError types
• Migrates gix-date to use gix_error::ParseError instead of custom error enum
• Updates gix-revision to return Result<(), Exn> from delegate methods instead of Option<()>
• Updates gix crate's revision parsing to handle new error types and update test snapshots
• Updates gix-refspec, gix-actor, and gitoxide-core for compatibility

Reviewed changes

Copilot reviewed 70 out of 71 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
gix-error/*	New crate providing error handling primitives with call-site tracking
gix-date/*	Migrated to use gix_error::ParseError
gix-revision/*	Updated delegate traits to return Result<(), Exn> instead of Option<()>
gix/src/revision/spec/parse/*	Extensive refactoring to use new error types
gix/tests/*	Updated test snapshots for new error messages
gix-refspec/*	Updated to handle boxed ParseError
Cargo.toml, Cargo.lock	Added gix-error to workspace

[EliahKagan]

The failure here, in the windows-latest job for test-fast, is:

        FAIL [   1.502s] ( 337/2966) gix::gix revision::spec::from_bytes::bad_objects_are_valid_until_they_are_actually_read_from_the_odb
  stdout ───

    running 1 test
    ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
    Snapshot: bad_objects_are_valid_until_they_are_actually_read_from_the_odb-2
    Source: D:\a\gitoxide\gitoxide:145
    ───────────────────────────────────────────────────────────────────────────────
    Expression: format!("{err:#?}").replace('\\', "/")
    ───────────────────────────────────────────────────────────────────────────────
    -old snapshot
    +new results
    ────────────┬──────────────────────────────────────────────────────────────────
        1     1 │ delegate.peel_until(ValidObject) failed: {object}
        2     2 │ |
        3     3 │ └─ An error occurred while obtaining an object from the loose object store
        4     4 │ |
        5       │-└─ decompression of loose object at 'tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/2990428670-unix/blob.corrupt/objects/ca/fea31147e840161a1860c50af999917ae1536b' failed
              5 │+└─ decompression of loose object at 'tests/fixtures/generated-do-not-edit/make_rev_spec_parse_repos/2990428670-windows/blob.corrupt/objects/ca/fea31147e840161a1860c50af999917ae1536b' failed
        6     6 │ |
        7     7 │ └─ Could not decode zip stream, status was 'Invalid input data'
        8     8 │ |
        9     9 │ └─ Invalid input data
    ────────────┴──────────────────────────────────────────────────────────────────
    Stopped on the first failure. Run `cargo insta test` to run all snapshots.
    test revision::spec::from_bytes::bad_objects_are_valid_until_they_are_actually_read_from_the_odb ... FAILED

    failures:

    failures:
        revision::spec::from_bytes::bad_objects_are_valid_until_they_are_actually_read_from_the_odb

    test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 338 filtered out; finished in 1.49s
    
  stderr ───

    thread 'revision::spec::from_bytes::bad_objects_are_valid_until_they_are_actually_read_from_the_odb' (2524) panicked at C:\Users\runneradmin\.cargo\registry\src\index.crates.io-1949cf8c6b5b557f\insta-1.46.0\src\runtime.rs:719:13:
    snapshot assertion for 'bad_objects_are_valid_until_they_are_actually_read_from_the_odb-2' failed in line 145
    note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

(This appeared after auto-merge was enabled, and blocks it, suggesting it may be unexpected.)

[EliahKagan]

@Byron Also, is the recent switch from merging with a merge commit to using "rebase and merge" intentional? That's what auto-merge is curently set to do here, and it looks like it's been used in a couple other places, yet very rarely until recently. I ask because:

1. We have more "sub-PRs" now than in the past. It used to be that I was the only one who liked making PRs on PRs 😄, and even I rarely did it. But now it's far more common because that's what happens when one uses @copilot in an existing PR to request that Copilot make a change that is meant to go in it. On "sub-PRs" themselves, the "rebase and merge" strategy can be cleaner since one avoids merge commits that don't represent integration points with the main branch.

   The GitHub web-based interface automatically defaults to whichever of "Merge," "Rebase and merge," or "Squash and merge" has most recently been selected (per user, per target repository). So when using it to integrate sub-branches into feature branches, it's easy to then inadvertently use it to integrate feature branches into the main branch, even when not intended.

   (This may furthermore be especially unintended: if "Rebase and merge" is used on "sub-PRs" to make it so that integration points into main correspond to merge commits, then omitting merge commits for those integration points keeps that goal from being achieved.)

2. If this is intentional and the goal is to have a more linear history, then there may be some other measures we should take to help with that.

   One is that I should know about this preference so I can try doing it myself. I do often try to keep the history linear by rebasing my own PRs before then doing a merge-commit merge with them, but not always.

   The other, though, is that we can set up a merge queue where GitHub automatically takes care of rebasing first as needed to keep the history linear. This feature works together with branch protection rulesets and required checks, it can be more robust, and I believe it can be used equally well with a merge-commit or "Rebase and merge" preference (i.e., we can get a linear history and a merge commit too, all without having to rebase manually).

   I'd be interested to look into setting up the merge queue feature here, if having a more linear history is a goal.

[Byron]

OMG! Finally!

It took me ours to find a silly bug which would make downcasting fail, a critical feature to be able to inspect the error tree. Now that it's proven to be working via resolve_revspec, I can finally merge this and take everything else one step at a time.

The punch went through!

@Byron Also, is the recent switch from merging with a merge commit to using "rebase and merge" intentional? That's what auto-merge is curently set to do here, and it looks like it's been used in a couple other places, yet very rarely until recently.

I only intended to use it for Copilot sub-PRs, which I tried to use more here. The idea was to keep it separate, but bring it in as flat commit list so I could still rebase the parent PR when needed.
For bringing in PRs, I am still a fan of merge commits.

The GitHub web-based interface automatically defaults to whichever of "Merge," "Rebase and merge," or "Squash and merge" has most recently been selected (per user, per target repository). So when using it to integrate sub-branches into feature branches, it's easy to then inadvertently use it to integrate feature branches into the main branch, even when not intended.

This is exactly what happened 😅.

So there is no change overall, just me failing to change the setting back. Good it didn't merge :D.

[Byron]

With that said, I really like Exn, or what it has become :D. One will have to put more thought into how to represent errors as there are multiple options now, but on the flip-side, when can finally inline simple error messages while still building an error chain with callsite information.

It still needs anyhow integration to make the error sequential and turn it into a proper source() chain, but that's just the cherry on top (albeit a much needed cherry). Maybe into_box() could take care of that and then anyhow is basically free. into_any_box() would probably do it.

But I digress, I haven't been this excited in a while.