can you fix the conflict and rebase onto main? · Entire

What happened

The rebase of soph/http-stale-pool-fix (8 commits) onto origin/main conflicted in go.mod and go.sum — both on the go-git version line:

I kept the pseudo-version, because the pseudo-version's alpha.4.0.<ts>-<commit> form means its commit (go-git PR #2142, which adds WithObjectSelector) lands after the alpha.4 tag. The streaming-push code in internal/gitproto/push.go calls WithObjectSelector, so the tagged alpha.4 wouldn't compile — confirmed by the commit message ("Will switch to a tagged release once one is cut").

Verification

Notes

Both comments are still relevant — one strongly, one arguably

1. "Cancel masked as HTTP 401" (auth.go) — ✅ still valid

The interface was extended since the review: Lookup now returns an err, and tryHelperRetry does propagate it (smarthttp.go:451-455 returns the error instead of falling through). So the plumbing to fix this now exists — but the implementation still doesn't use it:

output, helperErr := GitCredentialCommand(ctx, CredentialOpFill, input)
if helperErr != nil {
    return "", "", false, nil //nolint:nilerr // helper failure means "no credentials available"
}

GitCredentialCommand runs via exec.CommandContext(ctx, …), so a cancelled/timed-out context kills the subprocess and returns an error — which line 111 blanket-swallows into ok=false, err=nil. The //nolint:unparam // err is always nil today on line 100 confirms it's never populated. Net effect is exactly what the bot described: a cancel/timeout during git credential fill surfaces as the original HTTP 401 rather than context.Canceled/DeadlineExceeded.

Fix is now small and localized to Lookup: when helperErr != nil && ctx.Err() != nil, return ctx.Err() instead of swallowing.

2. "Premature credential helper approve" (smarthttp.go) — ⚠️ structurally still present, but low severity

The logic moved into tryHelperRetry (smarthttp.go:471-473) and the new probe path resolvePendingHelperCreds (:399-407). Both still call Approve + set c.Auth on any 2xx — and in RequestInfoRefs that runs at lines 199-205, beforehttpError (208), the content-type check (219-221), and the size-limit check (236-238). So a 2xx with the wrong content-type/oversized body still persists credentials while RequestInfoRefs returns an error.

My take, for nodo's "I think this is valid right?": it's real but low severity, and I'd push back slightly on the "leak" framing:

If you want strict "approve only on a fully-validated success," the fix is to move resolvePendingHelperCreds

Recommendation: Fix #1 (it's a genuine, now-easy diagnostics bug). For #2, either resolve the thread with a note that 2xx == auth-success-by-design, or do the small reorder if you prefer strictness.

Want me to make either change? Note this is the soph/issue-63-deferred-credential-helper branch (PR #65), not the one currently checked out (soph/http-stale-pool-fix), so I'd need to switch branches or use a worktree first.

Branch is ready: 9 commits ahead of the old remote (8 rebased + 1 new fix), 8 behind because of the SHA changes from the rebase. Want me to git push --force-with-lease?

Summary of what's in the new commit (5ec540df)

Root cause: Go's http.Client.shouldCopyHeaderOnRedirect strips Authorization on cross-host redirects, so when an origin redirected to a challenger, our retry-against-c.EndpointURL replayed through the same redirect and arrived at the challenger naked — got 401 — and we Reject'd the user's valid creds.

Two code paths fixed:

  1. tryHelperRetry — captures res.Request.URL (the actually-challenged URL after redirects) and passes it as an override target to the retry callback. The retry hits the challenger directly, no redirect, auth intact.
  2. EnsureAuthForService — probe-then-Lookup-against-challengeURL (previously Lookup-then-probe-then-key-on-something-else, which led to Approve/Reject hitting a key the helper has no entry for). Drops the pre-probe Lookup optimization — necessary trade-off since the user may have creds for the challenger and not for the origin.

Signature changes: doInfoRefsRequest and doPostRPCRequest gained an optional target *url.URL; nil = old behaviour.

Tests added:

Test update: TestRun_Replicate_SubcommandExecutesAgainstEmptyTarget now expects 2 receive-pack POSTs (probe + real) since the probe is unconditional; TestMain comment in cmd/git-sync updated.

Summary of what changed

1. No more EndpointURL mutation. A new c.resolvedEndpoint field carries the post-redirect scheme/host when one's been discovered; c.EndpointURL stays exactly as the caller passed it in. URL-building reads c.requestURL(), which returns resolvedEndpoint when set and EndpointURL otherwise. The user-typed URL is now stable for display, logging, telemetry, the SSH-scheme check in refs.go, and the github.com check in bootstrap.go — none of which want the resolved value.