Skip to content

protocol: split async and blocking APIs for fetching refmaps#2274

Merged
Sebastian Thiel (Byron) merged 13 commits intoGitoxideLabs:mainfrom
djc:simplify-ls-refs
Dec 7, 2025
Merged

protocol: split async and blocking APIs for fetching refmaps#2274
Sebastian Thiel (Byron) merged 13 commits intoGitoxideLabs:mainfrom
djc:simplify-ls-refs

Conversation

@djc
Copy link
Copy Markdown
Contributor

@djc Dirkjan Ochtman (djc) commented Nov 26, 2025

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).

Copy link
Copy Markdown
Member

@Byron Sebastian Thiel (Byron) left a 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.

@djc
Copy link
Copy Markdown
Contributor Author

Tests should be passing now.

@Byron
Copy link
Copy Markdown
Member

Thanks a lot, I will take a look soon and see to merging it.
Meanwhile, I am unleashing an AI reviewer to see what it can do, please feel free to ignore trash or to ignore it entirely.

Copy link
Copy Markdown

Copilot AI left a 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 LsRefsCommand struct with separate invoke_async and invoke_blocking methods
  • Created FetchRefMap enum to represent either a fetched refmap or a command awaiting execution
  • Updated Handshake::fetch_or_extract_refmap to return FetchRefMap instead of directly fetching
  • Removed the old ls_refs() function and Action enum 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

@djc
Copy link
Copy Markdown
Contributor Author

AI code review comments were pretty good, addressed all of them.

Copy link
Copy Markdown

Copilot AI left a 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.

@Byron Sebastian Thiel (Byron) merged commit eab774c into GitoxideLabs:main Dec 7, 2025
34 checks passed
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