ref:6cddd4f7626bcde116d297bb8c7ce150b007642c

fix(protocol): add PktLine.decode_raw for binary sideband reassembly (#64)

SidebandWriterTest flaked on main: it reassembles a random binary payload (:crypto.strong_rand_bytes) via PktLine.decode, whose decode_one strips one trailing LF from every data packet. That's correct for text lines but wrong for binary — when a sideband chunk ends in 0x0A the byte is dropped and the reassembled payload no longer matches. Same root cause as the walker flake (#63, fixed in #39), different test file. The encode side is already split (encode adds LF for text; encode_raw / encode_sideband don't, for binary). Complete the symmetry on the decode side: add PktLine.decode_raw/1 (+ decode_one_raw/1) returning payloads verbatim, factoring the framing through a shared do_decode_one/2 so decode/1's text-stripping behavior is byte-identical. Use decode_raw in the sideband test helper. Audited the other sideband-decoding tests: upload_pack_test / upload_pack_v2_test / protocol_interop_test already use raw extract_sideband_pack helpers for the pack body, receive_pack_streaming decodes only text report-status — none flaky. Tests: decode_raw unit tests (preserves trailing LF + 0x0A frame boundaries); sideband_writer + full protocol suite green (107 tests); decode/1 text path unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
SHA: 6cddd4f7626bcde116d297bb8c7ce150b007642c
Author: CI <ci@anvil.test>
Date: 2026-05-29 07:22
Parents: ef39fe6
3 files changed +62 -18
Type
lib/ex_git_objectstore/protocol/pkt_line.ex +35 −13
@@ -106,25 +106,47 @@
@spec decode(binary()) :: {:ok, [packet()], binary()} | {:error, term()}
@type packet :: :flush | :delim | :response_end | {:data, binary()}
def decode(data), do: decode_loop(data, [])
def decode(data), do: decode_loop(data, [], :strip_lf)
@doc """
Decode pkt-line data WITHOUT stripping trailing LF from data payloads.
Use this for binary content — notably sideband pack data — where a payload
byte of `0x0A` is data, not a line terminator. `decode/1` strips a trailing
LF (the text-line convention, pairing with `encode/1`); `decode_raw/1`
returns payloads verbatim, pairing with `encode_raw/1` / `encode_sideband/2`.
"""
@spec decode_raw(binary()) :: {:ok, [packet()], binary()} | {:error, term()}
def decode_raw(data), do: decode_loop(data, [], :verbatim)
@doc """
Decode a single pkt-line from the front of the binary.
Returns `{:ok, packet, rest}` or `{:need_more, data}`.
"""
@spec decode_one(binary()) ::
{:ok, packet(), binary()} | {:need_more, binary()} | {:error, term()}
def decode_one(<<@flush, rest::binary>>), do: {:ok, :flush, rest}
def decode_one(<<@delim, rest::binary>>), do: {:ok, :delim, rest}
def decode_one(<<@response_end, rest::binary>>), do: {:ok, :response_end, rest}
def decode_one(data), do: do_decode_one(data, :strip_lf)
def decode_one(<<hex_len::binary-size(4), _rest::binary>> = data) do
@doc """
Like `decode_one/1` but returns the data payload verbatim (no trailing-LF
strip), for binary/sideband frames. See `decode_raw/1`.
"""
@spec decode_one_raw(binary()) ::
{:ok, packet(), binary()} | {:need_more, binary()} | {:error, term()}
def decode_one_raw(data), do: do_decode_one(data, :verbatim)
defp do_decode_one(<<@flush, rest::binary>>, _mode), do: {:ok, :flush, rest}
defp do_decode_one(<<@delim, rest::binary>>, _mode), do: {:ok, :delim, rest}
defp do_decode_one(<<@response_end, rest::binary>>, _mode), do: {:ok, :response_end, rest}
defp do_decode_one(<<hex_len::binary-size(4), _rest::binary>> = data, mode) do
case Integer.parse(hex_len, 16) do
{len, ""} when len >= 4 and len <= @max_pkt_len ->
if byte_size(data) >= len do
<<_hex::binary-size(4), payload::binary-size(len - 4), rest::binary>> = data
# Strip exactly one trailing LF if present (spec says receivers should strip it)
payload = strip_one_trailing_lf(payload)
# Text frames strip a trailing LF (spec convention, pairs with
# encode/1); binary/sideband frames (:verbatim) keep every byte.
payload = if mode == :strip_lf, do: strip_one_trailing_lf(payload), else: payload
{:ok, {:data, payload}, rest}
else
{:need_more, data}
@@ -138,8 +160,8 @@
end
end
def decode_one(data) when byte_size(data) < 4, do: {:need_more, data}
def decode_one(_), do: {:error, :invalid_pkt_line}
defp do_decode_one(data, _mode) when byte_size(data) < 4, do: {:need_more, data}
defp do_decode_one(_data, _mode), do: {:error, :invalid_pkt_line}
@doc """
Split sideband data (band 1 = pack data, band 2 = progress, band 3 = error).
@@ -184,15 +206,15 @@
# -- Private --
defp decode_loop(<<>>, acc, _mode), do: {:ok, Enum.reverse(acc), <<>>}
defp decode_loop(<<>>, acc), do: {:ok, Enum.reverse(acc), <<>>}
defp decode_loop(data, acc) do
case decode_one(data) do
defp decode_loop(data, acc, mode) do
case do_decode_one(data, mode) do
{:ok, :flush, rest} ->
{:ok, Enum.reverse([:flush | acc]), rest}
{:ok, packet, rest} ->
decode_loop(rest, [packet | acc], mode)
decode_loop(rest, [packet | acc])
{:need_more, rest} ->
{:ok, Enum.reverse(acc), rest}
test/ex_git_objectstore/protocol/pkt_line_test.exs +22 −0
@@ -118,6 +118,28 @@
end
end
describe "decode_raw" do
test "preserves a trailing LF that decode/1 would strip" do
# "0006a\n" = 4-byte len (0006) + payload "a\n".
data = "0006a\n"
assert {:ok, [{:data, "a\n"}], ""} = PktLine.decode_raw(data)
# decode/1 (text) strips it; decode_raw/1 (binary) keeps it.
assert {:ok, [{:data, "a"}], ""} = PktLine.decode(data)
end
test "reassembles binary sideband payloads byte-for-byte, incl. 0x0A boundaries" do
# Two channel-1 frames whose chunks end in 0x0A — the case that made
# the LF-stripping decode drop bytes.
frames =
PktLine.encode_sideband_frame(1, <<1, 2, 0x0A>>) <>
PktLine.encode_sideband_frame(1, <<3, 0x0A>>)
{:ok, packets, ""} = PktLine.decode_raw(frames)
payload = for({:data, <<1, rest::binary>>} <- packets, do: rest) |> IO.iodata_to_binary()
assert payload == <<1, 2, 0x0A, 3, 0x0A>>
end
end
describe "roundtrip" do
test "encode then decode" do
original = "some data here"
test/ex_git_objectstore/protocol/sideband_writer_test.exs +5 −5
@@ -29,7 +29,7 @@
defp decode_sideband_payload(frames) do
# Concatenate the payloads from every sideband-1 frame in `frames`.
{:ok, packets, <<>>} = PktLine.decode(frames)
{:ok, packets, <<>>} = PktLine.decode_raw(frames)
packets
|> Enum.map(fn
@@ -39,6 +39,6 @@
end
defp count_frames(frames) do
{:ok, packets, <<>>} = PktLine.decode_raw(frames)
{:ok, packets, <<>>} = PktLine.decode(frames)
length(packets)
end
@@ -60,7 +60,7 @@
frames = collect(fn sb -> SidebandWriter.write(sb, payload) end)
{:ok, packets, <<>>} = PktLine.decode(frames)
{:ok, packets, <<>>} = PktLine.decode_raw(frames)
# Two full frames + one tail frame.
assert length(packets) == 3
assert decode_sideband_payload(frames) == payload
@@ -83,7 +83,7 @@
assert decode_sideband_payload(frames) == expected
max = PktLine.max_sideband_data()
{:ok, packets, <<>>} = PktLine.decode_raw(frames)
{:ok, packets, <<>>} = PktLine.decode(frames)
for {:data, <<_band, chunk::binary>>} <- packets do
assert byte_size(chunk) <= max
@@ -99,7 +99,7 @@
Enum.reduce(chunks, sb, &SidebandWriter.write(&2, &1))
end)
{:ok, packets, <<>>} = PktLine.decode(frames)
{:ok, packets, <<>>} = PktLine.decode_raw(frames)
# First write fills a frame exactly (drained), second is a short final frame.
assert length(packets) == 2
assert decode_sideband_payload(frames) == IO.iodata_to_binary(chunks)