Skip to content

Commit

Permalink
fix: assure we don't stop early on NAK if READY was sent to handle V1…
Browse files Browse the repository at this point in the history
… specialty (#882).

Seeing READY means a pack will follow, which is exactly the kind of knowledge we want.
Thus we now either listen to the client OR to what the server says.

Note that this remedy relies on `multi-ack-detailed`, which some very old servers might
not support. For those we would be out-of-luck, but that seems acceptable.
  • Loading branch information
Byron committed Jun 7, 2023
1 parent 28d2cf5 commit 88670c2
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 5 deletions.
9 changes: 6 additions & 3 deletions gix-protocol/src/fetch/response/async_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ impl Response {
let mut line = String::new();
let mut acks = Vec::<Acknowledgement>::new();
let mut shallows = Vec::<ShallowUpdate>::new();
let mut saw_ready = false;
let has_pack = 'lines: loop {
line.clear();
let peeked_line = match reader.peek_data_line().await {
Expand Down Expand Up @@ -81,14 +82,16 @@ impl Response {
if Response::parse_v1_ack_or_shallow_or_assume_pack(&mut acks, &mut shallows, &peeked_line) {
break 'lines true;
}
if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack) {
break 'lines false;
}
assert_ne!(
reader.readline_str(&mut line).await?,
0,
"consuming a peeked line works"
);
// When the server sends ready, we know there is going to be a pack so no need to stop early.
saw_ready |= matches!(acks.last(), Some(Acknowledgement::Ready));
if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack && !saw_ready) {
break 'lines false;
}
};
Ok(Response {
acks,
Expand Down
7 changes: 5 additions & 2 deletions gix-protocol/src/fetch/response/blocking_io.rs
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ impl Response {
let mut line = String::new();
let mut acks = Vec::<Acknowledgement>::new();
let mut shallows = Vec::<ShallowUpdate>::new();
let mut saw_ready = false;
let has_pack = 'lines: loop {
line.clear();
let peeked_line = match reader.peek_data_line() {
Expand Down Expand Up @@ -81,10 +82,12 @@ impl Response {
if Response::parse_v1_ack_or_shallow_or_assume_pack(&mut acks, &mut shallows, &peeked_line) {
break 'lines true;
}
if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack) {
assert_ne!(reader.readline_str(&mut line)?, 0, "consuming a peeked line works");
// When the server sends ready, we know there is going to be a pack so no need to stop early.
saw_ready |= matches!(acks.last(), Some(Acknowledgement::Ready));
if let Some(Acknowledgement::Nak) = acks.last().filter(|_| !client_expects_pack && !saw_ready) {
break 'lines false;
}
assert_ne!(reader.readline_str(&mut line)?, 0, "consuming a peeked line works");
};
Ok(Response {
acks,
Expand Down

0 comments on commit 88670c2

Please sign in to comment.