Review PR 128 And Fix Issues

Claude Code·Opus 5.5·

Soph·1w ago·6hr 9min·2 Checkpoints·5 file changes·+126/-20·17.3K tokens

I reviewed PR #128. The main problem is a test that fails: go test ./internal/gitproto/ fails every time on the PR branch on your machine and passes on main. I checked out the PR head temporarily and removed it afterwards. Nothing was posted to the PR.

Correctness

The new ordering breaks the cancellation test (internal/gitproto/ssh.go:104). TestSSHConnPostRPCStreamBodyHonorsContext failed 10 out of 10 runs. Close returns write |1: broken pipe / signal: killed instead of context deadline exceeded.
- The PR now reads the advertisement before it starts writing the body. Here, shell startup takes longer than the test's 50ms deadline, so ssh is killed while the advertisement is still being read.
- discardSSHAdvertisement treats that EOF as "no advertisement" and reports success. The body is then written into a dead pipe.
- In real use, a fetch cancelled during a large advertisement reports a confusing broken-pipe error instead of the cancellation.
sshRPCStream.Close hides the cancellation (internal/gitproto/ssh.go:333). Its copy-error branch returns before it adds ctx.Err(). After the reorder, a copy error following cancellation is the normal case, so errors.Is(err, context.Canceled) comes back false.
closeJoin can fail fetches that worked (fetch.go:213 for v2, fetch.go:491 for v1). Any error from closing the stream now fails the fetch, even after the pack has been stored. Both the ssh and helper streams close their pipes before waiting for the process. If the server still has trailing bytes to send, or the helper exits non-zero when stdin closes, a good fetch fails with something like exit status 255. requestInfoRefsWithCommand already avoids reporting these teardown errors on purpose. This is unconfirmed: I couldn't check how git-remote-entire exits when stdin closes.
A sync can hang forever (ssh.go:104). The advertisement is now read before anything is written to stdin. A server or wrapper that waits for input before it advertises will block on Peek(4) while the server waits for stdin. Without a context deadline, the sync never finishes. The EOF and non-pkt-line branches of discardSSHAdvertisement were written to tolerate servers like that.
Cancellation during the advertisement isn't reported as cancellation (ssh.go:105). The new advertisement-error path doesn't add ctx.Err(), unlike requestInfoRefsWithCommand. A cancel that interrupts a pkt-line comes back as a pkt-line read error.

Tests

The new regression test depends on timing (ssh_test.go:302). It relies on the old code writing within sleep 0.2. read -t 0 only sees data that has already arrived. On a slow CI runner, the old buggy ordering could also pass, so the test wouldn't catch the regression.

Cleanup

closeJoin mostly duplicates go-git's ioutil.CheckClose (fetch.go:218). CheckClose is still used in the same file, in FetchCommitParents (line 142). The actual bug was the unnamed return value, not CheckClose. The file now has two close-error helpers that behave differently: one keeps the first error, the other joins both.
Error output gets noisy (fetch.go:220). On the failure path, errors.Join adds a multi-line teardown error (exit status plus remote stderr) to every decode or update failure. It reads like a second failure.

Points 1 and 2 are the blocker and come from the same change. Point 3 is the one I'd confirm before merging, by checking how the helper exits when stdin closes. I can draft review comments, but I'll ask before posting anything to GitHub.

I checked each finding and committed fixes for the ones that held up. The commit 3c8e9852 sits on top of paul/ssh-empty-packfile in this worktree and isn't pushed yet. go test ./... and golangci-lint pass, and the SSH tests passed 5 out of 5 runs.

Why the test failed: on this Mac, running a brand-new script takes about 2s (most likely macOS scanning it before the first run). So the 50ms deadline kills the shim before it prints anything. That empty output is treated as "no advertisement", so the request body is then written into a dead pipe and fails with broken pipe. Close returned that error without the context error. On main the body was written into the pipe buffer before the kill, which is why the test passed there.

#	Finding	Verdict	Change
1	Advertisement-first order breaks the cancellation test	Real (cause above)	`PostRPCStreamBody` no longer sends the request if ctx is done after reading the advertisement. The test now accepts the deadline at either point, with a comment explaining why.
2	`sshRPCStream.Close` drops `ctx.Err()` when the body copy failed	Real	`Close` always adds `ctx.Err()`, which also simplified that function. New timing-free unit test.
3	`closeJoin` can fail a fetch that stored everything	Real for the helper	`git-remote-entire` writes its final `0002` as a separate write after the pack. If we close stdout first, the helper dies on SIGPIPE. On success, `closeJoin` now reads and discards up to 64 KiB of trailing bytes before closing. Covers v1 and v2; new tests for both the success and failure cases.
4	Server that waits for input before advertising could hang	Not a problem	`git-upload-pack` always advertises first, and reading the advertisement first is exactly what `git` does.
5	Cancellation during the advertisement not reported as cancellation	Real	Folded into fix 1, with its own test.
6	Regression test depends on timing	Minor	It can only fail to catch a regression on a slow machine; it never fails the new code. Left as is.
7	`closeJoin` duplicates `CheckClose`	Not a problem	`CheckClose` keeps only the first error, which would drop the remote's stderr that the PR is trying to surface. With the drain it's a different helper anyway.
8	Teardown errors make failure messages noisy	Left as is	Keeping the remote's stderr on failure is the point of the PR.

The CHANGELOG.md entries already cover this change, so I didn't add one.

Should I push to paul/ssh-empty-packfile? It's Paul's PR branch, so the commit would show up on his PR. The alternative is a separate branch and PR that targets his.