Skip to content

Conversation

@djc
Copy link
Contributor

@djc djc commented Oct 26, 2025

No description provided.

@djc djc force-pushed the additive-transport branch 2 times, most recently from 291d5fd to 0e377f7 Compare October 26, 2025 07:54
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 so much for contributing this!

I am pleasantly surprised how well this works, and it clearly was a mistake to not start additively in the first place. After all, on the consuming side, one can just import the same type name (either async or sync), and existing code will work as before.
So even if this for some reason stops translating up the dependency tree, there is a net-win.

This was just a cursory review, but I think once I can have my rustdoc links back, I can finish it and get this merged.

Comment on lines +4 to +7
#[cfg(feature = "async-network-client")]
use gix_transport::client::async_io::Transport;
#[cfg(feature = "blocking-network-client")]
use gix_transport::client::blocking_io::Transport;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am particularly interested to see how this is ever (of course not in this PR) becoming additive without code duplication. But maybe it doesn't have to be, as making the plumbing crates additive is already a win.
In any case, I am staying tuned 🤩.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think there will be a bit more duplication at the higher levels, hopefully that will still be acceptable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also realise now that as long as callers can use traits, these will work perfectly due to maybe_async. So maybe, just maybe, this will work just fine.

@djc djc force-pushed the additive-transport branch from 0e377f7 to c2050d0 Compare October 27, 2025 08:02
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.

Fantastic work! Even after looking carefully at everything, I wasn't able to make any of my usual refactoring 😁, so merging as is.

@Byron Byron merged commit 6e89afa into GitoxideLabs:main Oct 30, 2025
28 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