Deduplicate packetline crates #2220

djc · 2025-10-15T09:27:54Z

Trying to make a little progress on #2152 (and this copied crate stuff seems extremely ugly anyway).

Why does cargo check pass while cargo check -p gix break? Something to do with feature unification? This means I'm not getting feedback from VS Code on errors, which makes it much harder to get stuff done.

Byron · 2025-10-15T12:11:56Z

Thanks for giving it a shot, it's much appreciated!

Feature-unification is the reason for requiring a differently named copy. It's very ugly, but there was no other way.
It all goes down to wanting to be able to 'switch' async on or off with minimal code duplication and shared tests. This is how one atrocity led to another.
Like mentioned before, I'd live better without async entirely (in this codebase).

djc · 2025-10-15T12:14:08Z

Feature-unification is the reason for requiring a differently named copy. It's very ugly, but there was no other way.
It all goes down to wanting to be able to 'switch' async on or off with minimal code duplication and shared tests. This is how one atrocity led to another.

Do you have reason to think the approach in this PR would not work? It seems like an existence proof contradicting that "there was no other way" (and I don't think it introduces substantial code duplication, although the tests might be a bit harder).

Byron · 2025-10-15T12:25:11Z

Apologies, I didn't even look at the code yet and wanted to wait until you think it's ready for review.
My statement was about how this came to be, it's not the wish to torture users, but to solve a specific problem that led down this path.
Sometimes other solutions aren't seen in time leading to suboptimal results, and I genuinely hope that this PR can show the way.

djc · 2025-10-15T12:34:09Z

Apologies, I didn't even look at the code yet and wanted to wait until you think it's ready for review.

It obviously doesn't pass CI yet, but I'd like some early feedback before I try to tackle the remaining issues. I think the idea is conceptually simple: the gix-packetline crate offers both async and sync APIs, separately (both are toggled by a Cargo feature flag), and consumers explicitly use the API for the mode they want to use. I don't think there's substantially more library-level code duplication. For now, this leaves mutually exclusive features in higher-level crates alone, at some point the I/O mode will bubble up to the top-level API and then we have to decide how to best present that.

Byron

I think I understand now what this PR is going for and have a feeling that it could work. Maybe gix-packetline never needed have offer one unified codepath that can be 'toggled' async due to its primitive nature.

Now that I think about it, maybe the tests for gix-packetline then will be more duplicated, but that should be alright as they are stable.

And yes, I agree that from here more opportunities for similar updates may show up.

gix-filter/Cargo.toml

djc · 2025-10-18T21:19:09Z

This feels like it's pretty close. Somehow some builds in the full tests end up enabling both async-client and blocking-client on gix-transport which are supposed to be mutually exclusive on this branch as before -- I'm not sure what's triggering that? It doesn't feel like any of the changes I made should be capable of that, they're all supposed to be upstream of gix-transport.

djc-2021 dedup-packetline gitoxide $ cargo check --features lean-async
    Checking gix-packetline v0.19.1 (/Users/djc/src/gitoxide/gix-packetline)
    Checking gix-filter v0.20.0 (/Users/djc/src/gitoxide/gix-filter)
    Checking gix-transport v0.48.0 (/Users/djc/src/gitoxide/gix-transport)
error: Cannot set both 'blocking-client' and 'async-client' features as they are mutually exclusive
  --> gix-transport/src/lib.rs:92:1
   |
92 | compile_error!("Cannot set both 'blocking-client' and 'async-client' features as they are mutually exclusive");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

djc · 2025-10-19T07:42:55Z

This feels like it's pretty close. Somehow some builds in the full tests end up enabling both async-client and blocking-client on gix-transport which are supposed to be mutually exclusive on this branch as before -- I'm not sure what's triggering that? It doesn't feel like any of the changes I made should be capable of that, they're all supposed to be upstream of gix-transport.
djc-2021 dedup-packetline gitoxide $ cargo check --features lean-async
    Checking gix-packetline v0.19.1 (/Users/djc/src/gitoxide/gix-packetline)
    Checking gix-filter v0.20.0 (/Users/djc/src/gitoxide/gix-filter)
    Checking gix-transport v0.48.0 (/Users/djc/src/gitoxide/gix-transport)
error: Cannot set both 'blocking-client' and 'async-client' features as they are mutually exclusive
  --> gix-transport/src/lib.rs:92:1
   |
92 | compile_error!("Cannot set both 'blocking-client' and 'async-client' features as they are mutually exclusive");
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Ahh, I think I just didn't understand the justfile ! prefix syntax. I think this next run might pass CI. Honestly not sure why it's useful to have CI check that certain configurations don't compile.

djc · 2025-10-19T20:33:51Z

I've tacked on a second commit (which can be reviewed independently) to clean up the gix-packetline API in a way that seems quite a bit nicer to me.

Byron

Great news! I see that this PR can be merged, I just need some more time with it, and didn't yet have time for my refactoring round.

Generally, I'd wish API improvements would be in a separate commit.
TCC

gix-packetline/src/async_io/encode.rs

Byron · 2025-10-22T04:45:35Z

gix-packetline/src/read.rs

I think this should be read/mod.rs.

Why? There are no nested modules, right? Putting it in a directory without a need for further nesting seems pretty unidiomatic.

That's true. To me it looked like there is read on the top-level, or does it not?

The view is a bit confusing, it shows a read directory because it was there previously and has now been deleted.

Byron · 2025-10-22T04:46:03Z

gix-packetline/src/lib.rs

This can be write.rs.

It's not clear what this is referring to.

Byron · 2025-10-22T04:47:18Z

gix-packetline/src/lib.rs

+#[cfg(feature = "blocking-io")]
+pub mod blocking_io {


That's cool, I don't think I have ever seen this kind of style before. I will take some time to wrap my head around it :).

djc · 2025-10-22T05:55:55Z

and didn't yet have time for my refactoring round.

What do you mean by this?

Byron · 2025-10-22T06:27:06Z

This means I have only done a partial armchair review without actually opening the code in an editor. When I do, I usually just make the changes I need and merge. That saves a lot of time as I don't have to go back and forth.

djc · 2025-10-22T12:00:50Z

Rebased onto current main.

djc force-pushed the dedup-packetline branch from 480fa19 to ea41222 Compare October 15, 2025 10:52

Byron marked this pull request as draft October 15, 2025 12:09

Byron reviewed Oct 17, 2025

View reviewed changes

gix-filter/Cargo.toml Outdated Show resolved Hide resolved

djc force-pushed the dedup-packetline branch 3 times, most recently from c791c05 to 9c3977c Compare October 18, 2025 21:12

djc marked this pull request as ready for review October 18, 2025 21:14

djc force-pushed the dedup-packetline branch from 9c3977c to 6cfaf8d Compare October 18, 2025 21:17

djc force-pushed the dedup-packetline branch 4 times, most recently from f9607bf to 4ea089f Compare October 19, 2025 07:42

Byron reviewed Oct 22, 2025

View reviewed changes

djc added 2 commits October 22, 2025 14:00

Deduplicate packetline crates

c185ce8

packetline: simplify API

ad90ced

djc force-pushed the dedup-packetline branch from d66725c to ad90ced Compare October 22, 2025 12:00

Uh oh!

Deduplicate packetline crates #2220

Are you sure you want to change the base?

Deduplicate packetline crates #2220

Conversation

djc commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc commented Oct 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Byron commented Oct 15, 2025

Uh oh!

djc commented Oct 15, 2025

Uh oh!

Byron left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

djc commented Oct 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc commented Oct 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

djc commented Oct 19, 2025

Uh oh!

Byron left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Byron Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

djc Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Byron Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

AaronDewes Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Byron Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

djc Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

Byron Oct 22, 2025

Choose a reason for hiding this comment

Uh oh!

djc commented Oct 22, 2025

Uh oh!

Byron commented Oct 22, 2025

Uh oh!

djc commented Oct 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

djc commented Oct 15, 2025 •

edited

Loading

Byron commented Oct 15, 2025 •

edited

Loading

djc commented Oct 15, 2025 •

edited

Loading

Byron left a comment •

edited

Loading

djc commented Oct 18, 2025 •

edited

Loading

djc commented Oct 19, 2025 •

edited

Loading