Skip to content

Deduplicate packetline crates #2220

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+650 −3,372
Open

Deduplicate packetline crates #2220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 0 additions & 36 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -540,41 +540,6 @@ jobs:
- name: gix-pack with all features (including wasm)
run: cargo build -p gix-pack --all-features --target "$TARGET"

check-packetline:
strategy:
fail-fast: false
matrix:
os:
- ubuntu-latest
# We consider this script read-only and its effect is the same everywhere.
# However, when changes are made to `etc/scripts/copy-packetline.sh`, re-enable the other platforms for testing.
# - macos-latest
# - windows-latest

runs-on: ${{ matrix.os }}

defaults:
run:
# Use `bash` even on Windows, if we ever reenable `windows-latest` for testing.
shell: bash

steps:
- uses: actions/checkout@v5
with:
persist-credentials: false
- name: Check that working tree is initially clean
run: |
set -x
git status
git diff --exit-code
- name: Regenerate gix-packetline-blocking/src
run: etc/scripts/copy-packetline.sh
- name: Check that gix-packetline-blocking/src was already up to date
run: |
set -x
git status
git diff --exit-code
# Check that all `actions/checkout` in CI jobs have `persist-credentials: false`.
check-no-persist-credentials:
runs-on: ubuntu-latest
Expand Down Expand Up @@ -655,7 +620,6 @@ jobs:
- test-32bit-windows-size-doc
- lint
- cargo-deny
- check-packetline
- check-no-persist-credentials
- check-blocking

Expand Down
14 changes: 1 addition & 13 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,6 @@ members = [
"gix-status",
"gix-revision",
"gix-packetline",
"gix-packetline-blocking",
"gix-mailmap",
"gix-macros",
"gix-note",
Expand Down
152 changes: 0 additions & 152 deletions etc/scripts/copy-packetline.sh
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion gix-filter/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ gix-command = { version = "^0.6.2", path = "../gix-command" }
gix-quote = { version = "^0.6.0", path = "../gix-quote" }
gix-utils = { version = "^0.3.0", path = "../gix-utils" }
gix-path = { version = "^0.10.20", path = "../gix-path" }
gix-packetline-blocking = { version = "^0.19.1", path = "../gix-packetline-blocking" }
gix-packetline = { version = "^0.19.1", path = "../gix-packetline", features = ["blocking-io"] }
Copy link
Member

Choose a reason for hiding this comment

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

And this is what can't be happening. Setting this to blocking sets it to blocking for everyone in the dependency tree, even though there maybe others who want it async.

This PR solves this by making gix-packetline features additive, which might be OK to adjust here assuming that the code that uses it already is clearly split into sync and async.

If that's the case, I think it makes total sense to continue this effort.

Copy link
Contributor

@lorenzleutgeb lorenzleutgeb Oct 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In principle, if there are two features "gix-packetline/blocking-io" and "gix-packetline/async-io" (which seems to be the case IIUC), and these are additive, this could be fine.

However, it'd be good to understand why this can't be:

[features]
blocking-io = ["gix-packetline/blocking-io"]
[dependencies]
gix-packetline = { version = "...", path = "...", features = [] } # for emphasis

That is, why can't gix-filter also have a feature toggle on its I/O, and propagate down to gix-packetline?
If this is impossible, then this must imply that gix-filter somehow intrinsically requires blocking I/O. And then this would be the next best frontier to push, i.e. to work on getting gix-filter to support both blocking and async. Or you decide that this is not worth doing and just declare gix-filter to require blocking I/O, full stop. You could future-proof by requiring the feature flag "blocking-io" for the crate to compile at all (like gix-hash now requires "sha1"), just to make room for a potential "async-io" down the line.

So long as this is not mutually exclusive with enabling async I/O, this might just be a minor annoyance...

Copy link
Author

Choose a reason for hiding this comment

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

On main, gix-filter unconditionally depends on gix-packetline-blocking, which I take to mean that gix-filter indeed depends on the blocking I/O implementation in gix-packetline. Solving that problem is not my goal for now -- allowing the features not to be mutually exclusive is.

gix-attributes = { version = "^0.27.0", path = "../gix-attributes" }

encoding_rs = "0.8.32"
Expand Down
29 changes: 15 additions & 14 deletions gix-filter/src/driver/process/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ pub mod invoke {
#[error("Failed to read or write to the process")]
Io(#[from] std::io::Error),
#[error(transparent)]
PacketlineDecode(#[from] gix_packetline_blocking::decode::Error),
PacketlineDecode(#[from] gix_packetline::decode::Error),
}

impl From<super::Error> for Error {
Expand All @@ -65,18 +65,19 @@ impl Client {
versions: &[usize],
desired_capabilities: &[&str],
) -> Result<Self, handshake::Error> {
let mut out =
gix_packetline_blocking::Writer::new(process.stdin.take().expect("configured stdin when spawning"));
let mut out = gix_packetline::write::blocking_io::Writer::new(
process.stdin.take().expect("configured stdin when spawning"),
);
out.write_all(format!("{welcome_prefix}-client").as_bytes())?;
for version in versions {
out.write_all(format!("version={version}").as_bytes())?;
}
gix_packetline_blocking::encode::flush_to_write(out.inner_mut())?;
gix_packetline::encode::blocking_io::flush_to_write(out.inner_mut())?;
out.flush()?;

let mut input = gix_packetline_blocking::StreamingPeekableIter::new(
let mut input = gix_packetline::read::blocking_io::StreamingPeekableIter::new(
process.stdout.take().expect("configured stdout when spawning"),
&[gix_packetline_blocking::PacketLineRef::Flush],
&[gix_packetline::PacketLineRef::Flush],
false, /* packet tracing */
);
let mut read = input.as_read();
Expand Down Expand Up @@ -126,10 +127,10 @@ impl Client {
for capability in desired_capabilities {
out.write_all(format!("capability={capability}").as_bytes())?;
}
gix_packetline_blocking::encode::flush_to_write(out.inner_mut())?;
gix_packetline::encode::blocking_io::flush_to_write(out.inner_mut())?;
out.flush()?;

read.reset_with(&[gix_packetline_blocking::PacketLineRef::Flush]);
read.reset_with(&[gix_packetline::PacketLineRef::Flush]);
let mut capabilities = HashSet::new();
loop {
buf.clear();
Expand Down Expand Up @@ -168,7 +169,7 @@ impl Client {
) -> Result<process::Status, invoke::Error> {
self.send_command_and_meta(command, meta)?;
std::io::copy(content, &mut self.input)?;
gix_packetline_blocking::encode::flush_to_write(self.input.inner_mut())?;
gix_packetline::encode::blocking_io::flush_to_write(self.input.inner_mut())?;
self.input.flush()?;
Ok(self.read_status()?)
}
Expand All @@ -190,15 +191,15 @@ impl Client {
inspect_line(line.as_bstr());
}
}
self.out.reset_with(&[gix_packetline_blocking::PacketLineRef::Flush]);
self.out.reset_with(&[gix_packetline::PacketLineRef::Flush]);
let status = self.read_status()?;
Ok(status)
}

/// Return a `Read` implementation that reads the server process output until the next flush package, and validates
/// the status. If the status indicates failure, the last read will also fail.
pub fn as_read(&mut self) -> impl std::io::Read + '_ {
self.out.reset_with(&[gix_packetline_blocking::PacketLineRef::Flush]);
self.out.reset_with(&[gix_packetline::PacketLineRef::Flush]);
ReadProcessOutputAndStatus {
inner: self.out.as_read(),
}
Expand Down Expand Up @@ -226,7 +227,7 @@ impl Client {
buf.push_str(&value);
self.input.write_all(&buf)?;
}
gix_packetline_blocking::encode::flush_to_write(self.input.inner_mut())?;
gix_packetline::encode::blocking_io::flush_to_write(self.input.inner_mut())?;
Ok(())
}
}
Expand All @@ -249,7 +250,7 @@ fn read_status(read: &mut PacketlineReader<'_>) -> std::io::Result<process::Stat
if count > 0 && matches!(status, process::Status::Previous) {
status = process::Status::Unset;
}
read.reset_with(&[gix_packetline_blocking::PacketLineRef::Flush]);
read.reset_with(&[gix_packetline::PacketLineRef::Flush]);
Ok(status)
}

Expand All @@ -261,7 +262,7 @@ impl std::io::Read for ReadProcessOutputAndStatus<'_> {
fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
let num_read = self.inner.read(buf)?;
if num_read == 0 {
self.inner.reset_with(&[gix_packetline_blocking::PacketLineRef::Flush]);
self.inner.reset_with(&[gix_packetline::PacketLineRef::Flush]);
let status = read_status(&mut self.inner)?;
if status.is_success() {
Ok(0)
Expand Down
15 changes: 6 additions & 9 deletions gix-filter/src/driver/process/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,9 +12,9 @@ pub struct Client {
/// The negotiated version of the protocol.
version: usize,
/// A way to send packet-line encoded information to the process.
input: gix_packetline_blocking::Writer<std::process::ChildStdin>,
input: gix_packetline::write::blocking_io::Writer<std::process::ChildStdin>,
/// A way to read information sent to us by the process.
out: gix_packetline_blocking::StreamingPeekableIter<std::process::ChildStdout>,
out: gix_packetline::read::blocking_io::StreamingPeekableIter<std::process::ChildStdout>,
}

/// A handle to facilitate typical server interactions that include the handshake and command-invocations.
Expand All @@ -24,9 +24,9 @@ pub struct Server {
/// The negotiated version of the protocol, it's the highest supported one.
version: usize,
/// A way to receive information from the client.
input: gix_packetline_blocking::StreamingPeekableIter<std::io::StdinLock<'static>>,
input: gix_packetline::read::blocking_io::StreamingPeekableIter<std::io::StdinLock<'static>>,
/// A way to send information to the client.
out: gix_packetline_blocking::Writer<std::io::StdoutLock<'static>>,
out: gix_packetline::write::blocking_io::Writer<std::io::StdoutLock<'static>>,
}

/// The return status of an [invoked command][Client::invoke()].
Expand Down Expand Up @@ -109,8 +109,5 @@ pub mod client;
///
pub mod server;

type PacketlineReader<'a, T = std::process::ChildStdout> = gix_packetline_blocking::read::WithSidebands<
'a,
T,
fn(bool, &[u8]) -> gix_packetline_blocking::read::ProgressAction,
>;
type PacketlineReader<'a, T = std::process::ChildStdout> =
gix_packetline::read::blocking_io::WithSidebands<'a, T, fn(bool, &[u8]) -> gix_packetline::read::ProgressAction>;
Loading
Loading