ref:624b9d54e5ee9bd2ad647e8bb1c431195046729b

fix: UploadPackV2 emits `ready` (or omits acks) when client sends `done`

`git fetch` against a repo served by `UploadPackV2` failed with fatal: expected no other sections to be sent after no 'ready' when the client's local history was unrelated to what's on the server. The server emitted `acknowledgments\n NAK\n 0001 packfile\n ...`, which violates the protocol v2 grammar — real git clients reject any acknowledgments section that doesn't end with `ready` when a packfile section follows. Parse `done` from the fetch args and build the acks section accordingly: - `done` + no matching haves -> omit the acks section - `done` + some ACKs -> emit `ACK ... / ready / delim` - no `done` (multi-round negotiation) -> emit `NAK / flush` or `ACK ... / flush` Adds two test layers: - state-machine unit tests that lock in the protocol invariant - a real-`git`-client interop test that spins up an in-process `git://` TCP daemon and runs `git clone` / `git fetch` against UploadPackV2 (catches framing bugs pure-Elixir tests miss) Closes #27 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SHA: 624b9d54e5ee9bd2ad647e8bb1c431195046729b
Author: Cole Christensen <cole.christensen@macmillan.com>
Date: 2026-04-18 22:21
Parents: 2b54c14
3 files changed +441 -17
Type
lib/ex_git_objectstore/protocol/upload_pack_v2.ex +36 −14
@@ -225,11 +225,14 @@
defp handle_fetch(repo, args) do
wants = extract_shas(args, "want ")
haves = extract_shas(args, "have ")
done = Enum.any?(args, &(String.trim(&1) == "done"))
Logger.info(
"UploadPackV2.handle_fetch: #{length(wants)} wants, #{length(haves)} haves, done=#{done}"
)
Logger.info("UploadPackV2.handle_fetch: #{length(wants)} wants, #{length(haves)} haves")
# Build acknowledgments section when client sends haves
ack_section = build_acknowledgments(repo, haves, done)
ack_section = build_acknowledgments(repo, haves)
case collect_objects(repo, wants, haves) do
{:ok, objects} ->
@@ -254,13 +257,13 @@
end
end
defp build_acknowledgments(_repo, []) do
# No haves = initial clone, no acknowledgments section needed
defp build_acknowledgments(_repo, [], _done) do
# No haves = initial clone, no acknowledgments section needed.
<<>>
end
defp build_acknowledgments(repo, haves, done) do
# Check which haves we have in common.
defp build_acknowledgments(repo, haves) do
# Check which haves we have in common
acks =
haves
|> Enum.filter(fn sha ->
@@ -271,16 +274,35 @@
end)
|> Enum.map(fn sha -> PktLine.encode("ACK #{sha}") end)
header = PktLine.encode("acknowledgments")
# 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.
# 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.
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 found — tell the client to send more haves.
acks == [] ->
header = PktLine.encode("acknowledgments")
ack_lines =
if acks == [] do
[PktLine.encode("NAK")]
else
acks ++ [PktLine.encode("ready")]
end
IO.iodata_to_binary([header, PktLine.encode("NAK"), PktLine.flush()])
true ->
IO.iodata_to_binary([header | ack_lines] ++ [PktLine.delim()])
header = PktLine.encode("acknowledgments")
IO.iodata_to_binary([header | acks] ++ [PktLine.flush()])
end
end
defp extract_shas(args, prefix) do
test/ex_git_objectstore/integration/upload_pack_v2_git_client_test.exs +317 −0
@@ -1,0 +1,317 @@
# Copyright 2026 Cole Christensen
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
defmodule ExGitObjectstore.Integration.UploadPackV2GitClientTest do
@moduledoc """
End-to-end validation of UploadPackV2 against the real `git` CLI.
Each test spins up a minimal TCP server that speaks the `git://`
protocol (service line + protocol-v2 stream) and serves an
in-memory repository through `UploadPackV2`. A subprocess invocation
of `git clone` / `git fetch` is then run against `127.0.0.1:<port>`
and its exit code + stderr are asserted.
This catches protocol framing bugs that pure state-machine tests
miss, because a real client strictly enforces the v2 response
grammar (e.g. `acknowledgments` section must end with `ready` if
a `packfile` section follows).
"""
use ExUnit.Case, async: false
alias ExGitObjectstore.Object
alias ExGitObjectstore.Object.{Blob, Commit, Tree}
alias ExGitObjectstore.Protocol.{PktLine, UploadPackV2}
alias ExGitObjectstore.Ref
alias ExGitObjectstore.Test.RepoHelper
@moduletag :integration
@moduletag timeout: :timer.minutes(1)
# --- fixtures ---
defp create_commit(repo, content, message, parents \\ []) do
blob = Blob.from_content(content)
{:ok, blob_sha} = Object.write(repo, blob)
tree = Tree.new([%{mode: "100644", name: "file.txt", sha: blob_sha}])
{:ok, tree_sha} = Object.write(repo, tree)
commit = %Commit{
tree: tree_sha,
parents: parents,
author: "Test <t@t.com> 1000000000 +0000",
committer: "Test <t@t.com> 1000000000 +0000",
message: message
}
{:ok, commit_sha} = Object.write(repo, commit)
commit_sha
end
# --- tests ---
describe "git clone against UploadPackV2 over git://" do
@tag :tmp_dir
test "clone of a single-commit repo succeeds", %{tmp_dir: tmp_dir} do
repo = RepoHelper.memory_repo("clone-single")
ExGitObjectstore.init(repo)
sha = create_commit(repo, "hello\n", "initial\n")
:ok = Ref.put(repo, "refs/heads/main", sha, nil)
{port, stop} = start_git_daemon(repo)
try do
dest = Path.join(tmp_dir, "clone")
{out, code} = git_clone("git://127.0.0.1:#{port}/repo", dest)
assert code == 0, "git clone failed (exit #{code}):\n#{out}"
assert File.exists?(Path.join(dest, "file.txt"))
after
stop.()
end
end
@tag :tmp_dir
test "clone of a multi-commit chain succeeds", %{tmp_dir: tmp_dir} do
repo = RepoHelper.memory_repo("clone-chain")
ExGitObjectstore.init(repo)
{_commits, _} =
Enum.reduce(1..5, {[], nil}, fn i, {acc, parent} ->
parents = if parent, do: [parent], else: []
sha = create_commit(repo, "v#{i}\n", "commit #{i}\n", parents)
if i == 1 do
:ok = Ref.put(repo, "refs/heads/main", sha, nil)
else
:ok = Ref.put(repo, "refs/heads/main", sha, parent)
end
{[sha | acc], sha}
end)
{port, stop} = start_git_daemon(repo)
try do
dest = Path.join(tmp_dir, "clone")
{out, code} = git_clone("git://127.0.0.1:#{port}/repo", dest)
assert code == 0, "git clone failed (exit #{code}):\n#{out}"
# Should have 5 commits on main
{log, _} = System.cmd("git", ["-C", dest, "log", "--oneline"], stderr_to_stdout: true)
assert length(String.split(String.trim(log), "\n")) == 5
after
stop.()
end
end
end
describe "git fetch against UploadPackV2 over git://" do
# Regression: hephaestus failed with
# fatal: expected no other sections to be sent after no 'ready'
# 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.
@tag :tmp_dir
test "fetch works when client has unrelated history (hephaestus regression)",
%{tmp_dir: tmp_dir} do
# 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"])
git!(client_dir, ["config", "user.email", "t@t.com"])
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"])
{port, stop} = start_git_daemon(repo)
try do
{out, code} =
System.cmd(
"git",
["-c", "protocol.version=2", "fetch", "git://127.0.0.1:#{port}/repo", "main"],
cd: client_dir,
stderr_to_stdout: true,
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}"
assert code == 0, "git fetch failed (exit #{code}):\n#{out}"
after
stop.()
end
end
@tag :tmp_dir
test "fetch after clone is a no-op (matching haves)", %{tmp_dir: tmp_dir} do
repo = RepoHelper.memory_repo("fetch-noop")
ExGitObjectstore.init(repo)
sha = create_commit(repo, "a\n", "a\n")
:ok = Ref.put(repo, "refs/heads/main", sha, nil)
{port, stop} = start_git_daemon(repo)
try do
dest = Path.join(tmp_dir, "clone")
{out, code} = git_clone("git://127.0.0.1:#{port}/repo", dest)
assert code == 0, "git clone failed (exit #{code}):\n#{out}"
# Fetch again — client has the tip, should negotiate and receive
# an empty (or nearly empty) pack.
{out2, code2} =
System.cmd(
"git",
["-c", "protocol.version=2", "fetch", "origin", "main"],
cd: dest,
stderr_to_stdout: true,
env: [{"GIT_TERMINAL_PROMPT", "0"}]
)
assert code2 == 0, "git fetch no-op failed (exit #{code2}):\n#{out2}"
after
stop.()
end
end
end
# --- helpers ---
defp git!(dir, args) do
{out, code} = System.cmd("git", args, cd: dir, stderr_to_stdout: true)
if code != 0, do: raise("git #{Enum.join(args, " ")} failed: #{out}")
String.trim(out)
end
defp git_clone(url, dest) do
System.cmd(
"git",
["-c", "protocol.version=2", "clone", url, dest],
stderr_to_stdout: true,
env: [{"GIT_TERMINAL_PROMPT", "0"}]
)
end
# Spin up a minimal git:// daemon serving one repo over UploadPackV2.
# Returns `{port, stop_fn}`.
defp start_git_daemon(repo) do
{:ok, listen} =
:gen_tcp.listen(0, [
:binary,
active: false,
reuseaddr: true,
packet: :raw,
ip: {127, 0, 0, 1}
])
{:ok, port} = :inet.port(listen)
# Unlinked: stopping the daemon (closing the listen socket) must not take
# the test process with it.
spawn(fn -> accept_loop(listen, repo) end)
stop = fn -> :gen_tcp.close(listen) end
{port, stop}
end
defp accept_loop(listen, repo) do
case :gen_tcp.accept(listen, 10_000) do
{:ok, client} ->
spawn(fn -> serve_client(client, repo) end)
accept_loop(listen, repo)
{:error, :closed} ->
:ok
{:error, _} = err ->
err
end
end
# Each connection = one git:// session.
# Flow:
# 1. Client sends a single pkt-line:
# `git-upload-pack <path>\0host=<h>\0[\0version=2\0]`
# 2. If version=2, we send the v2 capability advertisement and then
# forward subsequent bytes into UploadPackV2 until the state machine
# reports done.
defp serve_client(client, repo) do
with {:ok, service_line} <- read_one_pkt(client),
true <- v2_requested?(service_line) do
{advert, state} = UploadPackV2.init(repo)
:ok = :gen_tcp.send(client, advert)
drive(client, state)
else
_ -> :ok
end
after
:gen_tcp.close(client)
end
defp drive(client, state) do
case :gen_tcp.recv(client, 0, 15_000) do
{:ok, data} ->
{response, new_state} = UploadPackV2.feed(state, data)
if byte_size(response) > 0, do: :ok = :gen_tcp.send(client, response)
if UploadPackV2.done?(new_state) do
:ok
else
drive(client, new_state)
end
{:error, _} ->
:ok
end
end
# Read exactly one pkt-line from the socket. Returns `{:ok, payload}` or
# `{:error, reason}`.
defp read_one_pkt(client) do
with {:ok, <<hex::binary-size(4)>>} <- :gen_tcp.recv(client, 4, 5_000),
{len, ""} <- Integer.parse(hex, 16) do
read_pkt_payload(client, len)
else
:error -> {:error, :bad_pkt_len}
{_, _} -> {:error, :bad_pkt_len}
err -> err
end
end
defp read_pkt_payload(_client, 0), do: {:ok, ""}
defp read_pkt_payload(_client, len) when len < 4, do: {:error, :bad_pkt_len}
defp read_pkt_payload(client, len) do
:gen_tcp.recv(client, len - 4, 5_000)
end
defp v2_requested?(line) do
# git://-style service line contains NUL-separated args; v2 clients
# include `version=2` as one of them.
String.contains?(line, "version=2")
end
# quiet unused warning on future tweaks
_ = PktLine
end
test/ex_git_objectstore/protocol/upload_pack_v2_test.exs +88 −3
@@ -310,6 +310,51 @@
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.
repo = RepoHelper.memory_repo()
ExGitObjectstore.init(repo)
{c1_sha, _, _} = create_commit(repo, "v1\n", "first\n")
:ok = Ref.put(repo, "refs/heads/main", c1_sha, nil)
{_advert, state} = UploadPackV2.init(repo)
fake_have = String.duplicate("f", 40)
client_data =
PktLine.encode("command=fetch") <>
PktLine.delim() <>
PktLine.encode("want #{c1_sha}") <>
PktLine.encode("have #{fake_have}") <>
PktLine.encode("done") <>
PktLine.flush()
{response, _state} = UploadPackV2.feed(state, client_data)
refute String.contains?(response, "acknowledgments"),
"with `done` and no matching haves, the acks section must be omitted"
refute String.contains?(response, "NAK")
assert String.contains?(response, "packfile")
end
# Regression: hephaestus `git fetch` failed with
# fatal: expected no other sections to be sent after no 'ready'
# when the client's haves didn't match anything on the server but the
# client sent `done`.
#
# Per protocol v2 spec (Documentation/technical/protocol-v2.txt):
# acknowledgments = PKT-LINE("acknowledgments" LF) (nak | *ack) [ready]
# and:
# "If the server has found a common base commit, or the client has
# sent a `done` line, the server will send `ready` followed by the
# `packfile` section."
#
# So once `done` is in the request, the acks section must either be
# omitted entirely OR end with "ready" before any packfile.
test "with `done` and no matching haves: acks+packfile response must include `ready`" do
test "returns NAK when no common ancestors" do
repo = RepoHelper.memory_repo()
ExGitObjectstore.init(repo)
@@ -318,7 +363,47 @@
:ok = Ref.put(repo, "refs/heads/main", c1_sha, nil)
{_advert, state} = UploadPackV2.init(repo)
fake_have = String.duplicate("f", 40)
client_data =
PktLine.encode("command=fetch") <>
PktLine.delim() <>
PktLine.encode("want #{c1_sha}") <>
PktLine.encode("have #{fake_have}") <>
PktLine.encode("done") <>
PktLine.flush()
{response, _state} = UploadPackV2.feed(state, client_data)
has_acks = String.contains?(response, "acknowledgments")
has_packfile = String.contains?(response, "packfile")
has_ready = String.contains?(response, "ready")
# If both sections are sent, acks MUST contain "ready" — otherwise
# the git client errors with:
# fatal: expected no other sections to be sent after no 'ready'
if has_acks and has_packfile do
assert has_ready,
"Protocol v2 violation: response contains an `acknowledgments` section " <>
"followed by a `packfile` section, but the acks section does NOT include " <>
"`ready`. A real git client rejects this response with " <>
"`fatal: expected no other sections to be sent after no 'ready'`.\n\n" <>
"Response bytes:\n#{inspect(response, limit: :infinity, printable_limit: 2000)}"
end
end
test "with `done` and no matching haves: packfile is sent (client wants unconditional send)" do
# Sanity check — the `done` line means "stop negotiating and send what
# you've got". The server MUST send a packfile section. This test locks
# in that behaviour so a fix that removes the packfile instead of adding
# `ready` doesn't regress clone-from-scratch semantics.
repo = RepoHelper.memory_repo()
ExGitObjectstore.init(repo)
{c1_sha, _, _} = create_commit(repo, "v1\n", "first\n")
:ok = Ref.put(repo, "refs/heads/main", c1_sha, nil)
{_advert, state} = UploadPackV2.init(repo)
fake_have = String.duplicate("f", 40)
client_data =
@@ -331,7 +416,7 @@
{response, _state} = UploadPackV2.feed(state, client_data)
assert String.contains?(response, "packfile"),
"Client sent `done` — server must send a packfile section."
assert String.contains?(response, "acknowledgments")
assert String.contains?(response, "NAK")
end
end