Skip to content

Conversation

@djc
Copy link
Contributor

@djc 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
Member

@Byron 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 djc force-pushed the simplify-ls-refs branch from af9e974 to bb5b99f Compare December 1, 2025 09:52
@djc
Copy link
Contributor Author

djc commented Dec 1, 2025

Tests should be passing now.

@Byron
Copy link
Member

Byron commented Dec 2, 2025

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

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 djc force-pushed the simplify-ls-refs branch from bb5b99f to 348a837 Compare December 3, 2025 12:37
@djc
Copy link
Contributor Author

djc commented Dec 3, 2025

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

@djc djc force-pushed the simplify-ls-refs branch from 348a837 to ab3cce4 Compare December 3, 2025 13:01
@Byron Byron self-assigned this Dec 7, 2025
Copy link

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 Byron enabled auto-merge December 7, 2025 18:21
@Byron 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.

2 participants