-
-
Notifications
You must be signed in to change notification settings - Fork 405
protocol: split async and blocking APIs for fetching refmaps #2274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Byron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for keeping these PR coming, it's much appreciated!
Regarding the CI failure, it can be reproduced with cargo test -p gix-protocol --features blocking-client.
There are a few more general questions that arose from a first relatively brief look.
|
Tests should be passing now. |
|
Thanks a lot, I will take a look soon and see to merging it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the gix-protocol crate to split async and blocking APIs for fetching refmaps, removing reliance on the maybe_async macro in favor of explicit method variants. The change introduces a builder pattern with LsRefsCommand and a FetchRefMap enum that defers execution until either fetch_async or fetch_blocking is called.
Key Changes
- Introduced
LsRefsCommandstruct with separateinvoke_asyncandinvoke_blockingmethods - Created
FetchRefMapenum to represent either a fetched refmap or a command awaiting execution - Updated
Handshake::fetch_or_extract_refmapto returnFetchRefMapinstead of directly fetching - Removed the old
ls_refs()function andActionenum in favor of new API structure
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| gix-protocol/src/ls_refs.rs | Replaced ls_refs() function with LsRefsCommand builder pattern, adding separate invoke_async and invoke_blocking methods |
| gix-protocol/src/handshake/mod.rs | Added FetchRefMap enum and refactored fetch_or_extract_refmap to defer execution with separate async/blocking fetch methods |
| gix-protocol/src/lib.rs | Updated public API exports from ls_refs function to LsRefsCommand |
| gix-protocol/src/fetch/refmap/init.rs | Removed RefMap::fetch() method and associated helper functions, moved logic to LsRefsCommand |
| gix-protocol/src/fetch/types.rs | Updated documentation references to point to new API |
| gix-protocol/src/fetch/negotiate.rs | Updated documentation references to point to new API |
| gix/src/remote/connection/ref_map.rs | Updated to use new two-step API with feature-gated fetch_async/fetch_blocking calls |
| gitoxide-core/src/pack/receive.rs | Updated to use new two-step API with feature-gated fetch_async/fetch_blocking calls |
| gix-protocol/tests/protocol/fetch/_impl.rs | Updated test delegate trait to use simplified action() method and local RefsAction enum, adapted fetch logic to use LsRefsCommand |
| gix-protocol/tests/protocol/fetch/mod.rs | Removed unused imports related to old ls_refs API |
| gix-protocol/tests/protocol/fetch/v2.rs | Updated error matching to reflect flatter error structure |
|
AI code review comments were pretty good, addressed all of them. |
…tch_or_extract_refmap()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
This is just one part of the problem in gix-protocol, but I'm happy that I was able to resolve it with only about +20 lines despite some duplication.
Happy to take feedback if you can articulate it (to save you on refactoring time).