@@ -7,6 +7,22 @@
## [Unreleased]
### Fixed
- **UploadPackV2: omit `acknowledgments` section when the client sends
  `done`.** Real `git fetch` (v2.53) was failing against production
  with `fatal: expected 'packfile', received 'acknowledgments'`. Per
  `fetch-pack.c`: when `send_fetch_request` writes a `done\n` pkt-line
  it returns `done_sent=1`, and the client state machine transitions
  directly from `FETCH_SEND_REQUEST` to `FETCH_GET_PACK` — bypassing
  `FETCH_PROCESS_ACKS`. `FETCH_GET_PACK` expects `[shallow-info]
  [wanted-refs] [packfile-uris] packfile` and rejects any leading
  acknowledgments section. The server now skips the acks section
  entirely whenever `done` is in the request. Regression coverage: a
  state-machine-level test in `UploadPackV2NegotiationTest` and a
  real-git-client multi-round test that forces `done` alongside a
  non-empty haves batch by driving the negotiator past `MAX_IN_VAIN`.
### Added
- **Full protocol-v2 capability coverage for UploadPackV2.** Capability

@@ -32,6 +32,24 @@
  require Logger
  # MapSet values inside the shallow-walk state map trigger dialyzer
  # `call_without_opaque` / `contract_with_opaque` warnings — dialyzer
  # infers different shapes for a MapSet depending on whether the
  # callsite just built one via `MapSet.new/0` or has updated one via
  # `MapSet.put/2`, and the shapes don't unify inside a map-update
  # expression. The walk itself is fine; the warnings are a type-system
  # limitation on composing opaque values through map-update syntax.
  @dialyzer {:no_opaque,
             [
               walk_shallow_loop: 2,
               step_shallow: 4,
               visit_shallow_commit: 4,
               record_walked_commit: 3,
               process_shallow_parents: 5,
               maybe_unshallow: 2,
               parent_walkable?: 2
             ]}
  alias ExGitObjectstore.{ObjectResolver, Ref, Repo}
  alias ExGitObjectstore.Object
  alias ExGitObjectstore.Object.{Blob, Commit, Tag, Tree}
@@ -378,25 +396,50 @@
         wants,
         haves,
         _args,
         _done?,
         done?,
         wait_for_done?,
         shallow_opts,
         filter_spec,
         send_packfile?
       ) do
    ack_section = build_acknowledgments(repo, haves, send_packfile?)
    cond do
      # Client sent `done`. Per git's v2 fetch-pack state machine
      # (fetch-pack.c, `send_fetch_request` returns done_sent=1), the
      # client goes directly from FETCH_SEND_REQUEST to FETCH_GET_PACK,
      # bypassing FETCH_PROCESS_ACKS. FETCH_GET_PACK expects
      # `[shallow-info] [wanted-refs] [packfile-uris] packfile` — an
      # acknowledgments section here triggers
      #   fatal: expected 'packfile', received 'acknowledgments'
      done? ->
        {response, stats} =
          build_packfile_response(repo, wants, haves, <<>>, shallow_opts, filter_spec)
        {{response, :done}, stats}
      # wait-for-done stalls negotiation until the client sends `done`.
      # Emit a NAK/ACK acks section with flush so the client loops.
      wait_for_done? ->
        ack_section = build_acknowledgments(repo, haves, :flush)
        {{ack_section, :command}, %{pack_bytes: 0, objects: 0}}
        {{ack_section, :done}, %{pack_bytes: 0, objects: 0}}
      # Shallow/filter single-round without `done` (very rare — the real
      # client sends `done` when haves run out, which is typical for a
      # fresh shallow clone). Client is still in FETCH_PROCESS_ACKS so
      # we emit acks ending with `ready` + delim, then shallow-info +
      # packfile in the same response. `ready` triggers the state
      # transition to FETCH_GET_PACK.
      send_packfile? ->
        ack_section = build_acknowledgments(repo, haves, :ready)
        {response, stats} =
          build_packfile_response(repo, wants, haves, ack_section, shallow_opts, filter_spec)
        {{response, :done}, stats}
      # Multi-round: client hasn't sent `done` yet. Return acks with
      # flush so the client sends another round of haves.
      true ->
        ack_section = build_acknowledgments(repo, haves, :flush)
        {{ack_section, :command}, %{pack_bytes: 0, objects: 0}}
    end
  end
@@ -442,27 +485,35 @@
    end
  end
  # Callers: `:ready` ends the section with `ready` + delim-pkt (packfile
  # follows); `:flush` ends with flush-pkt (client sends another round).
  # Callers MUST NOT emit an acks section at all when `done` was in the
  # request — see `do_handle_fetch` for the rationale.
  defp build_acknowledgments(_repo, [], :flush) do
  defp build_acknowledgments(_repo, [], _send_packfile?) do
    # Client sent no haves at all — this is an initial clone. The
    # acknowledgments section is optional per protocol v2 and the
    # client does not expect one in that case, so omit it.
    <<>>
    # Multi-round case with no haves is odd (clients with no haves
    # typically also send `done`, which takes the no-acks path). Emit
    # a bare NAK + flush so the client can loop if it somehow gets here.
    IO.iodata_to_binary([
      PktLine.encode("acknowledgments"),
      PktLine.encode("NAK"),
      PktLine.flush()
    ])
  end
  defp build_acknowledgments(repo, haves, send_packfile?) do
    # Client sent at least one have. Per protocol v2 the client enters
  defp build_acknowledgments(_repo, [], :ready) do
    # Shallow/packfile-follows path with no haves: emit a minimal acks
    # section with `ready` + delim so the client transitions to
    # FETCH_GET_PACK.
    IO.iodata_to_binary([
      PktLine.encode("acknowledgments"),
      PktLine.encode("ready"),
      PktLine.delim()
    ])
  end
  defp build_acknowledgments(repo, haves, terminator) do
    # Grammar (Documentation/gitprotocol-v2.txt):
    # its `process_acks` state and expects an `acknowledgments` section
    # from us — even if none of the haves match. Omitting the section
    # here triggers the client error
    #   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 ->
@@ -475,20 +526,16 @@
    header = PktLine.encode("acknowledgments")
    case terminator do
      :ready ->
        # Packfile follows; end with `ready` + delim-pkt.
    cond do
      # We're about to send a packfile, so the acks section must end
      # with `ready` (+ delim) so the client knows to read the
      # following section. `ready` with zero ACKs is spec-valid
      # (`*ack` is 0-or-more).
      send_packfile? ->
        IO.iodata_to_binary([header | acks] ++ [PktLine.encode("ready"), PktLine.delim()])
      :flush when acks == [] ->
      # Multi-round negotiation: client hasn't sent `done` yet. No matches
      # found — tell the client to send more haves.
      acks == [] ->
        # No common commits found yet — NAK + flush, client sends more haves.
        IO.iodata_to_binary([header, PktLine.encode("NAK"), PktLine.flush()])
      :flush ->
      true ->
        IO.iodata_to_binary([header | acks] ++ [PktLine.flush()])
    end
  end

@@ -253,14 +253,19 @@
        assert response.status == 200,
               "unexpected HTTP status #{response.status}"
        # Per git v2.53 fetch-pack.c: when the client sends `done`,
        # `send_fetch_request` returns done_sent=1 and the client state
        # transitions FETCH_SEND_REQUEST → FETCH_GET_PACK, bypassing
        # FETCH_PROCESS_ACKS. The response MUST therefore start with
        # `packfile` (or optional `shallow-info`) — NOT with an
        # acknowledgments section.
        refute String.contains?(response.body, "acknowledgments"),
               "server sent acks after `done` — real git client would reject this " <>
                 "with `expected 'packfile', received 'acknowledgments'` " <>
        # The response body MUST include the `acknowledgments` section
        # when haves were sent. Omitting it is the exact bug the last
        # fix over-corrected into.
        assert String.contains?(response.body, "acknowledgments"),
               "server omitted the acks section when haves were sent (body: #{inspect(response.body, limit: 200)})"
                 "(body: #{inspect(response.body, limit: 200)})"
        assert String.contains?(response.body, "packfile"),
               "server must still send a packfile with `done` (body: #{inspect(response.body, limit: 200)})"
               "server must send a packfile with `done` (body: #{inspect(response.body, limit: 200)})"
      after
        stop.()
      end

@@ -28,5 +28,6 @@
  alias ExGitObjectstore.Object
  alias ExGitObjectstore.Object.{Blob, Commit, Tree}
  alias ExGitObjectstore.Protocol.{PktLine, UploadPackV2}
  alias ExGitObjectstore.Ref
  alias ExGitObjectstore.Test.{GitDaemon, RepoHelper}
@@ -274,6 +275,119 @@
    end
  end
  describe "done + haves response shape (regression for hephaestus)" do
    # Per git v2 protocol (and v2.53 client in `fetch-pack.c`): when the
    # client sends `done` in a fetch request, its state machine goes
    # directly from `FETCH_SEND_REQUEST` to `FETCH_GET_PACK` — bypassing
    # `FETCH_PROCESS_ACKS`. `FETCH_GET_PACK` expects `[shallow-info]
    # [wanted-refs] [packfile-uris] packfile` and will die with
    # `fatal: expected 'packfile', received 'acknowledgments'` if the
    # server sends an acks section first.
    #
    # Before this fix, the server emitted `acknowledgments ... ready`
    # whenever `done` was sent WITH haves (the existing tests all happen
    # to trigger `done` with 0 haves, which took the "empty acks" branch
    # and hid the bug). This reproduces the hephaestus failure directly.
    test "fetch response starts with `packfile`, never `acknowledgments`, when client sends done + haves" do
      repo = RepoHelper.memory_repo("done-with-haves")
      ExGitObjectstore.init(repo)
      want_sha = simple_commit(repo, "f.txt", "hi\n", [])
      :ok = Ref.put(repo, "refs/heads/main", want_sha, nil)
      # Unknown haves (SHAs the server has never seen) — matches what
      # the real git client does after a repo history rewrite or
      # when negotiation walks into branches the server lacks.
      unknown_haves = [
        String.duplicate("a", 40),
        String.duplicate("b", 40),
        String.duplicate("c", 40)
      ]
      have_lines =
        unknown_haves
        |> Enum.map(fn sha -> PktLine.encode("have #{sha}") end)
        |> IO.iodata_to_binary()
      request =
        PktLine.encode("command=fetch") <>
          PktLine.delim() <>
          PktLine.encode("want #{want_sha}") <>
          have_lines <>
          PktLine.encode("done") <>
          PktLine.flush()
      {_advert, state} = UploadPackV2.init(repo)
      {response, _state} = UploadPackV2.feed(state, request)
      assert byte_size(response) > 0, "server sent empty response"
      first_pkt = first_payload_pkt(response)
      refute first_pkt == "acknowledgments",
             "server sent an acknowledgments section after a `done`-bearing request. " <>
               "Modern git (v2.53) skips FETCH_PROCESS_ACKS and expects the packfile " <>
               "section directly, dying with `expected 'packfile', received " <>
               "'acknowledgments'`.\nFirst pkt-line: #{inspect(first_pkt)}\n" <>
               "Full response (first 256 bytes, hex): " <>
               (response |> :binary.part(0, min(256, byte_size(response))) |> Base.encode16())
      # Positive assertion: first section must be `packfile` (or
      # `shallow-info` if shallow was requested, which it wasn't here).
      assert first_pkt == "packfile",
             "expected first section to be `packfile`, got #{inspect(first_pkt)}"
    end
    # Same scenario but driven through the real git client end-to-end:
    # forces a multi-round where the final round sends `done` with haves.
    # Uses a side branch with many unrelated commits to push in_vain past
    # MAX_IN_VAIN (256), which is how the hephaestus trace hits
    # `seen_ack && in_vain >= MAX_IN_VAIN` and emits `done` alongside a
    # non-empty haves batch.
    @tag :tmp_dir
    @tag :slow
    test "real git client: done alongside non-empty haves", %{tmp_dir: tmp_dir} do
      # Server has a small linear main branch.
      repo = fresh_repo("done-haves-real", [{"f.txt", "v1\n"}, {"f.txt", "v2\n"}])
      {port, stop} = GitDaemon.start_upload_pack(repo)
      try do
        client = Path.join(tmp_dir, "client")
        GitDaemon.seed_client_clone("git://127.0.0.1:#{port}/repo", client)
        # Fan out many unrelated side branches on the client. Each new
        # commit is an extra `have` for the negotiator. 300 > MAX_IN_VAIN
        # (256) so the client eventually emits `done` while still having
        # haves to flush in the same request.
        GitDaemon.git!(client, ["checkout", "-b", "side"])
        for i <- 1..300 do
          File.write!(Path.join(client, "s.txt"), "v#{i}\n")
          GitDaemon.git!(client, ["add", "s.txt"])
          GitDaemon.git!(client, ["commit", "-m", "side #{i}"])
        end
        {out, code} =
          GitDaemon.git_at(client, [
            "-c",
            "protocol.version=2",
            "-c",
            "fetch.negotiationAlgorithm=consecutive",
            "fetch",
            "origin",
            "main"
          ])
        refute String.contains?(out, "expected 'packfile'"),
               "client rejected server's acks-after-done response:\n#{out}"
        assert code == 0,
               "fetch failed (exit #{code}):\n#{out}"
      after
        stop.()
      end
    end
  end
  describe "concurrency" do
    # Two concurrent clones against the SAME upload-pack daemon port.
    # This forces a real accept-loop race: the daemon must spawn two
@@ -410,5 +524,40 @@
      end)
    repo
  end
  defp simple_commit(repo, filename, content, parents) do
    blob = Blob.from_content(content)
    {:ok, blob_sha} = Object.write(repo, blob)
    tree = Tree.new([%{mode: "100644", name: filename, sha: blob_sha}])
    {:ok, tree_sha} = Object.write(repo, tree)
    commit = %Commit{
      tree: tree_sha,
      parents: parents,
      author: "T <t@t.com> 1000000000 +0000",
      committer: "T <t@t.com> 1000000000 +0000",
      message: "commit\n"
    }
    {:ok, sha} = Object.write(repo, commit)
    sha
  end
  # Parse the first non-sideband pkt-line payload out of a raw server
  # response. Returns the payload string stripped of its trailing LF,
  # or `nil` if the response starts with a flush/delim/sideband packet.
  defp first_payload_pkt(response) do
    case PktLine.decode(response) do
      {:ok, packets, _rest} ->
        Enum.find_value(packets, fn
          {:data, data} -> String.trim_trailing(data, "\n")
          _ -> nil
        end)
      _ ->
        nil
    end
  end
end

@@ -294,29 +294,33 @@
      assert new_state.phase == :done
      # Should have acknowledgments section with ACK for c1
      assert String.contains?(response, "acknowledgments")
      assert String.contains?(response, "ACK #{c1_sha}")
      assert String.contains?(response, "ready")
      # Per git v2.53's fetch-pack.c state machine: when the client sends
      # `done`, `send_fetch_request` returns done_sent=1 and the state
      # transitions directly from FETCH_SEND_REQUEST to FETCH_GET_PACK —
      # BYPASSING FETCH_PROCESS_ACKS. So the server MUST NOT emit an
      # acknowledgments section; the response goes straight to
      # `[shallow-info] packfile`.
      refute String.contains?(response, "acknowledgments"),
             "acks section after `done` triggers `expected 'packfile', received 'acknowledgments'`"
      # Should have packfile section
      assert String.contains?(response, "packfile")
      pack_data = extract_sideband_pack(response)
      assert pack_data != nil
      assert {:ok, entries} = Reader.parse(pack_data)
      # Should only have new objects (c2's commit + tree + blob = 3)
      # Haves are still used server-side to trim the pack — only new
      # objects (c2's commit + tree + blob = 3) are included.
      assert length(entries) == 3
    end
    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'
    test "unmatched haves + done: response skips acks and sends packfile directly" do
      # Regression for the hephaestus failure. Real git client (v2.53)
      # with `done` in the request is in FETCH_GET_PACK state; sending
      # any acknowledgments section causes:
      #   fatal: expected 'packfile', received 'acknowledgments'
      #
      # The response MUST start with `packfile` (or an optional
      # shallow-info section, which is absent here).
      # With `done`, the section must end in `ready` (since a packfile
      # follows immediately after).
      repo = RepoHelper.memory_repo()
      ExGitObjectstore.init(repo)
@@ -338,14 +342,8 @@
      {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"
      assert String.contains?(response, "ready"),
             "with `done` and packfile to follow, acks must end with `ready`"
      refute String.contains?(response, "acknowledgments"),
             "server sent acks after `done`; real git client will reject this"
      assert String.contains?(response, "packfile")
    end