Review PR 129 and Fix SSH Shim

I've started the review of PR #129 in the background. It's checking the PR diff for correctness bugs, and I'll pass the findings on once it finishes.

PR #129 has one blocking problem: an existing SSH integration test now fails on the PR head. I ran go test ./internal/syncer -run TestRun_IntegrationSyncOverSSHShimV2 and it fails with list source refs: peek pktline: EOF. The ./internal/gitproto SSH tests pass. The other five points below come from reading the code; I didn't run anything for them.

1. The syncer SSH integration test is broken (internal/syncer/ssh_integration_test.go:35) The PR updated ssh_test.go but not this file.

• Its fake ssh shim skips only one -o pair. With the new -o SendEnv=GIT_PROTOCOL, it treats -o as the destination and SendEnv=GIT_PROTOCOL as the remote command, so the server prints nothing.
• The check at line 92 still looks for the old command text GIT_PROTOCOL='version=2' git-upload-pack, which no longer appears.

2. Protocol v2 stops working on ordinary self-hosted SSH servers (internal/gitproto/ssh.go:225) Stock OpenSSH only accepts LANG LC_* variables by default, so it drops GIT_PROTOCOL sent through SendEnv. Before this PR, the GIT_PROTOCOL= prefix on the remote command got v2 through on those servers. Now:

• auto quietly falls back to v1, which re-sends the full ref list on every request.
• --protocol v2 fails outright with "source did not negotiate protocol v2".

The CHANGELOG lists this change under Fixed and doesn't mention the regression. A possible fix: if a server answers v0/v1 to the SendEnv attempt, retry with the old prefix.

3. The SSH error fallback in auto mode has outlived its purpose (internal/gitproto/refs.go:102) It existed for hosts that rejected the env-prefixed command, and that command is gone. Servers that ignore GIT_PROTOCOL already fall back through the v0/v1 path. What's left is that any SSH error causes a second ssh connection. Auth failures, unknown hosts and timeouts each cost two handshakes, and the user sees the error twice. I'd remove the fallback or limit it to genuine protocol-rejection errors.

4. On Windows, an inherited GIT_PROTOCOL can slip through (internal/gitproto/ssh.go:232) sshEnv removes the variable with a case-sensitive match. Windows variable names ignore case, so a lowercase git_protocol survives. That breaks the PR's promise that the caller's environment can't force v2 on v1 calls.

5. The test helper needs git 2.28 or newer (internal/gitproto/ssh_test.go:189) It runs git init -b main, which older git doesn't support. On those machines the test fails instead of skipping. It also picks up the user's init.defaultBranch setting and hooks.

6. TestSSHRPCProtocolEnv doesn't cover the user's ssh_config (internal/gitproto/ssh_test.go:101) The test proves the variable is removed from the environment before ssh starts. It doesn't cover a user whose ssh_config has SendEnv GIT_PROTOCOL or SetEnv GIT_PROTOCOL=version=2. Those can still force v2 on a v1 request, and the v2 response would then be read as v1. Either pass -o SetEnv=GIT_PROTOCOL= or document the limitation.

My priorities: #1 blocks the merge, #2 is a real behaviour change that deserves at least a CHANGELOG note, #3 is worth cleaning up in this PR, and #4–#6 are minor.

The review checked out a local branch pr129 to run the tests; you can delete it with git branch -D pr129. Nothing was posted to GitHub. Should I post any of these as review comments on the PR?

It isn't broken on main, and CI on the PR isn't green. Two things got crossed:

• The failure is on the PR head only. On main the integration test works, because it still uses the old command layout. PR #129 adds a second -o pair and changes how the remote command is built. It updates internal/gitproto/ssh_test.go but not internal/syncer/ssh_integration_test.go, so this test only breaks with the PR's changes.
• The PR's test check is red. Lint, govulncheck, license-check and Bugbot passed, which may be why it looked green. The failing test job (run 40458812939) fails on exactly this test:

That's the same error I got running it locally on the PR head. The fix belongs in the PR:

• Make the fake ssh shim at ssh_integration_test.go:35 skip every -o pair, not just one.
• Change the check at line 92 to look for GIT_PROTOCOL=version=2 in the environment the shim receives, instead of in the remote command.

I went through all six findings. Two needed changes, which I've committed on a new local branch fix/pr129-review, one commit on top of Paul's d3c40929. The full go test ./... passes, go vet is clean, and golangci-lint reports 0 issues on internal/syncer. I haven't pushed anything.

1. Broken integration test — real, fixed. In internal/syncer/ssh_integration_test.go the fake ssh now skips every -o pair instead of just one. It also records the GIT_PROTOCOL value it receives from ssh's environment, and the v2 check looks for that instead of the old command prefix. TestRun_IntegrationSyncOverSSHShimV2 passes again; it's the test that was failing CI.

2. v2 lost on login-shell servers — real, documented rather than worked around. This is exactly how git itself behaves: it only uses SendEnv and never prefixes the command, so those servers get v1 from git too. Adding a fallback to the prefix would bring back the approach this PR removes and add a second negotiation path, which is a design call for Paul. I added a sentence to the CHANGELOG entry saying these servers now get v1 until AcceptEnv GIT_PROTOCOL is added to sshd.

3. SSH error fallback in auto mode — left as is. It was introduced on purpose (ade49598, 3b30f879), it covers any SSH error during the v2 attempt and not only the old "Invalid command" case, and it has its own tests in refs_test.go. The cost is an extra ssh connection and a doubled error message when the connection genuinely fails. Removing it would change behaviour beyond what this PR is about.

4. Windows case-sensitivity — not valid. .goreleaser only builds for darwin and linux.

5. git init -b needs git 2.28 — not valid. The same flag is already used in about ten existing tests (git_http_backend_test.go, ssh_docker_test.go, sha256convert_test.go).

6. SetEnv in the user's ssh_config — rejected. git has the same exposure. The suggested -o SetEnv=GIT_PROTOCOL= needs OpenSSH 7.8 or later, and older clients reject the unknown option, which would break every SSH sync on them.

I also deleted the leftover local pr129 branch.

Should I push the commit to paul/ssh-protocol-v2? The PR has "allow maintainer edits" turned off, but the branch is in entireio, so a push from you should still work. The alternative is to leave the commit local so you can send it to Paul as a suggestion.

I pushed the fix to paul/ssh-protocol-v2, moving it from d3c40929 to 162427f3. Paul's branch hadn't changed since I branched off it, so nothing of his was overwritten.

CI has started on the new commit, and the test check that failed before is still pending. The full test suite passed on my machine.

I didn't comment on the PR. Paul might want a short note explaining why issues 3–6 were left alone; I can write and post one if you want.

All tests pass and lint is clean. Committing the merge locally: