ref:b0352c4a236e6fdc329d4284d3113d469d582ccc

fix: UploadPackV2 always emits acks section when haves were sent

Follow-up to #27. The previous fix omitted the `acknowledgments` section when the client sent haves that didn't match and also sent `done`. Real git clients that sent any have enter `process_acks` and reject this with fatal: expected 'acknowledgments', received 'packfile' Rule: if the `haves` list is non-empty, we MUST emit the `acknowledgments` section. With `done`, it ends in `ready`; otherwise `NAK` / `ACK` + flush (multi-round). The prior integration test didn't catch this because the freshly-init'd git client sent zero haves, so the broken branch never fired. This commit forces haves via `--negotiation-tip=refs/heads/main` + `--depth=1` and adds an explicit `refute` on the regression error. The state-machine test is also tightened to require the acks section to be present (not just "if present, must end in ready"). Closes #28 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SHA: b0352c4a236e6fdc329d4284d3113d469d582ccc
Author: Cole Christensen <cole.christensen@macmillan.com>
Date: 2026-04-18 23:24
Parents: 624b9d5
3 files changed +74 -37
Type
lib/ex_git_objectstore/protocol/upload_pack_v2.ex +23 −21
@@ -258,12 +258,26 @@
end
defp build_acknowledgments(_repo, [], _done) do
# No haves = initial clone, no acknowledgments section needed.
# Client sent no haves at all (initial clone). The acknowledgments
# section is optional per protocol v2 and the client does not expect
# one in that case, so omit it.
<<>>
end
defp build_acknowledgments(repo, haves, done) do
# Client sent at least one have. Per protocol v2 the client enters its
# `process_acks` state after sending haves and expects an
# Check which haves we have in common.
# `acknowledgments` section from us — even if none of the haves match.
# Omitting the section here causes the client to error with
# fatal: expected 'acknowledgments', received 'packfile'
# and emitting a section that ends with `NAK` before a packfile causes
# fatal: expected no other sections to be sent after no 'ready'
#
# Grammar (Documentation/technical/protocol-v2.txt):
# acknowledgments = PKT-LINE("acknowledgments" LF) (nak | *ack) [ready]
# When `done` is in the request we must end with `ready` (packfile
# follows); otherwise we end with `NAK` / `ACK`s and a flush (client
# sends another round of haves).
acks =
haves
|> Enum.filter(fn sha ->
@@ -274,33 +288,21 @@
end)
|> Enum.map(fn sha -> PktLine.encode("ACK #{sha}") end)
# Per protocol v2 (Documentation/technical/protocol-v2.txt):
# acknowledgments = PKT-LINE("acknowledgments" LF) (nak | *ack) [ready]
# and: if the client sent `done` the server MUST be "ready" — either by
# emitting a `ready` line at the end of the acks section, or by omitting
# the section entirely. Sending `NAK` followed by a packfile is a
# protocol violation; real git clients reject it with
# fatal: expected no other sections to be sent after no 'ready'
cond do
# Client sent `done` but nothing matched — omit the acks section.
header = PktLine.encode("acknowledgments")
# A packfile will follow unconditionally.
done and acks == [] ->
<<>>
# Client sent `done` — end the acks section with `ready` so the
# following packfile section is expected by the client.
cond do
# Client sent `done` — packfile follows, so end with `ready`.
# `ready` is valid with zero ACKs (spec allows `*ack [ready]` where
# `*ack` is zero or more).
done ->
header = PktLine.encode("acknowledgments")
IO.iodata_to_binary([header | acks] ++ [PktLine.encode("ready"), PktLine.delim()])
# Multi-round negotiation: client hasn't sent `done` yet. No matches
# Multi-round negotiation: client hasn't sent `done` yet.
# No matches found — tell the client to send more haves.
# found — tell the client to send more haves.
acks == [] ->
header = PktLine.encode("acknowledgments")
IO.iodata_to_binary([header, PktLine.encode("NAK"), PktLine.flush()])
true ->
header = PktLine.encode("acknowledgments")
IO.iodata_to_binary([header | acks] ++ [PktLine.flush()])
end
end
test/ex_git_objectstore/integration/upload_pack_v2_git_client_test.exs +35 −9
@@ -123,17 +123,28 @@
# when the local clone had commits the server didn't know about, so the
# client sent haves that didn't match. The server emitted NAK + packfile
# which violates protocol v2.
# Regression 1: hephaestus reported
# fatal: expected no other sections to be sent after no 'ready'
# when the client sent haves that didn't match anything server-side but
# also sent `done`. The server was emitting `NAK` + packfile.
#
# Regression 2: the first fix over-corrected by OMITTING the acks
# section entirely when haves didn't match, triggering
# fatal: expected 'acknowledgments', received 'packfile'
# because a client that sent haves enters its `process_acks` state and
# insists on seeing the section header.
#
# The scenario below forces the failing path: `--negotiation-tip=main`
# makes git include the client-local commit as a `have`, guaranteed to
# not exist on the server.
@tag :tmp_dir
test "fetch works when client has unrelated history (hephaestus regression)",
test "fetch succeeds when client sends haves the server does not know",
%{tmp_dir: tmp_dir} do
repo = RepoHelper.memory_repo("fetch-stale-haves")
# Server repo — single commit
repo = RepoHelper.memory_repo("fetch-divergent")
ExGitObjectstore.init(repo)
server_sha = create_commit(repo, "server\n", "server commit\n")
:ok = Ref.put(repo, "refs/heads/main", server_sha, nil)
# Client repo with its own unrelated history — so fetching will send
# haves that the server does not know about.
client_dir = Path.join(tmp_dir, "client")
File.mkdir_p!(client_dir)
git!(client_dir, ["init", "--initial-branch=main"])
@@ -141,22 +152,37 @@
git!(client_dir, ["config", "user.name", "t"])
File.write!(Path.join(client_dir, "local.txt"), "local\n")
git!(client_dir, ["add", "local.txt"])
git!(client_dir, ["commit", "-m", "unrelated local commit"])
git!(client_dir, ["commit", "-m", "local-only commit"])
{port, stop} = start_git_daemon(repo)
try do
# `--depth=1` forces a shallow fetch which is always single-round
# with `done`. Combined with `--negotiation-tip=refs/heads/main`
# this guarantees the failing protocol path: haves sent, none match,
# done=true.
{out, code} =
System.cmd(
"git",
[
["-c", "protocol.version=2", "fetch", "git://127.0.0.1:#{port}/repo", "main"],
"-c",
"protocol.version=2",
"fetch",
"--depth=1",
"--negotiation-tip=refs/heads/main",
"git://127.0.0.1:#{port}/repo",
"main"
],
cd: client_dir,
stderr_to_stdout: true,
env: [{"GIT_TERMINAL_PROMPT", "0"}, {"GIT_TRACE_PACKET", "0"}]
env: [{"GIT_TERMINAL_PROMPT", "0"}]
)
refute String.contains?(out, "expected no other sections to be sent after no 'ready'"),
"hephaestus regression — server response violates protocol v2:\n#{out}"
"regression 1 — server sent NAK + packfile:\n#{out}"
refute String.contains?(out, "expected 'acknowledgments', received 'packfile'"),
"regression 2 — server omitted the acks section when haves were sent:\n#{out}"
assert code == 0, "git fetch failed (exit #{code}):\n#{out}"
after
test/ex_git_objectstore/protocol/upload_pack_v2_test.exs +16 −7
@@ -310,9 +310,13 @@
assert length(entries) == 3
end
test "no common ancestors + done: omits acks section and sends packfile" do
# Per protocol v2, when the client sends `done` the server must either
# end the acks section with `ready` or omit the section entirely.
# Here, with no matching haves, we omit it — the packfile follows.
test "no matching haves + done: acks section is present and ends with `ready`" do
# Client sent haves → client expects an `acknowledgments` section
# (git's v2 client enters process_acks whenever it sent at least one
# have). Omitting the section triggers:
# fatal: expected 'acknowledgments', received 'packfile'
#
# With `done`, the section must end in `ready` (since a packfile
# follows immediately after).
repo = RepoHelper.memory_repo()
ExGitObjectstore.init(repo)
@@ -334,9 +338,14 @@
{response, _state} = UploadPackV2.feed(state, client_data)
assert String.contains?(response, "acknowledgments"),
"client sent haves — server MUST emit the acknowledgments section"
refute String.contains?(response, "NAK"),
"with `done`, no NAK; must be `ready` instead"
refute String.contains?(response, "acknowledgments"),
assert String.contains?(response, "ready"),
"with `done` and packfile to follow, acks must end with `ready`"
"with `done` and no matching haves, the acks section must be omitted"
refute String.contains?(response, "NAK")
assert String.contains?(response, "packfile")
end