errors: address review — nil-safe Error, lease-path sentinel, doc caveats

655c91c→main·

nodo·1mo ago·6 files·+67 added/-10 removed

Follow-up to review feedback on the ErrTargetRefMoved / RefRejectedError API:

• RefRejectedError.Error() no longer nil-panics on an externally-constructed value (exported Ref/Reason, unexported err): it falls back to a formatted "ref X rejected: reason". Embedders can now build &RefRejectedError{...} in their own errors.As tests. Documented that the move classification can't be reproduced externally (moved is unexported) and to wrap the sentinel directly to exercise errors.Is.

• The BestEffort + ForceWithLease lease-failure escalation now also satisfies errors.Is(err, ErrTargetRefMoved) by wrapping the sentinel. Under explicit ForceWithLease the "ambiguous marker" caveat doesn't apply — the caller opted into lease semantics — so a lease miss there is definitionally a target move. Public doc notes this path is not itself a *RefRejectedError (prefer errors.Is over errors.As when you only need the cause).

• Documented that concurrentMoveMarkers is server-specific by design: "remote ref has changed" is entire-server's CAS reason (the case that matters); "stale info" is git's client-side force-with-lease phrasing kept for consistency; stock git servers phrase CAS misses differently ("failed to update ref" / "cannot lock ref ...") and are intentionally not matched. Extend as new server phrasings are observed.

• Documented that RefRejectedError reflects the first rejected ref when a push rejects several (report-status surfaces the first, per canonical git).

Changes

6

• MCHANGELOG.md+1/-1

• Merrors.go+8/-2

• internal

• gitproto

• Mpush.go+30/-6

• Mpush_test.go+15

• syncer

• Msyncer.go+6/-1

• Msyncer_test.go+7

Added

• Typed push-rejection errors on the public API. Sync/Replicate now report a *RefRejectedError (carrying the rejected Ref and the raw server Reason) for per-ref receive-pack ng statuses, reachable with errors.As. Rejections that are unambiguous concurrent target-ref moves — entire-server's compare-and-swap rejection (remote ref has changed) and git's --force-with-lease lease miss (stale info) — additionally satisfy errors.Is(err, ErrTargetRefMoved). This lets embedders distinguish a benign racing concurrent push (retryable) from a genuine push failure without substring-matching the free-form error message. Ambiguous markers (non-fast-forward / fetch first) are deliberately excluded from the move classification so a real "needs --force" rejection is not masked. The error message and the underlying *packp.CommandStatusErr are preserved unchanged, so existing checks keep working.

[0.6.0] - 2026-06-03

• MCHANGELOG.md+1/-1

// ErrTargetRefMoved is returned (wrapped) by Sync and Replicate when a push was // rejected because the target ref changed concurrently between this run's plan // and its push — a benign, retryable compare-and-swap / lease miss, not a real // failure. Test for it with errors.Is(err, gitsync.ErrTargetRefMoved). The // concrete error in the chain is a *RefRejectedError. // failure. Test for it with errors.Is(err, gitsync.ErrTargetRefMoved). // // On the default push path the concrete error in the chain is a // *RefRejectedError (reachable with errors.As). One case does NOT carry that // concrete type: a BestEffort run with ForceWithLease escalates a lease miss // through a plain wrapped error that still satisfies errors.Is(ErrTargetRefMoved) // but is not a *RefRejectedError. So prefer errors.Is when you only need the // cause, and treat a successful errors.As(*RefRejectedError) as best-effort. // // This is the supported way to distinguish a racing concurrent push from a // genuine push failure; prefer it over inspecting the error message text,

// satisfy errors.Is(err, ErrTargetRefMoved), letting callers branch on the cause // without substring-matching Reason themselves. Re-exported publicly as // gitsync.RefRejectedError. // // When a single push rejects multiple refs, report-status surfaces the first // failing ref (go-git follows canonical git here), so Ref/Reason reflect that // first ref; any others resurface on the next attempt. The ErrTargetRefMoved // classification cannot be reproduced by external construction (the deciding // field is unexported by design) — to exercise errors.Is in a downstream test, // wrap the sentinel directly: fmt.Errorf("...: %w", gitsync.ErrTargetRefMoved). type RefRejectedError struct { Ref string // the rejected ref, e.g. "refs/heads/main" Reason string // raw receive-pack ng reason, e.g. "remote ref has changed"

err   error // underlying error; preserves *packp.CommandStatusErr (+ any lease-hint annotation)

}

func (e *RefRejectedError) Error() string { return e.err.Error() } // Error is safe on a zero-value/externally-constructed RefRejectedError (one // with no wrapped err), so embedders can build &RefRejectedError{Ref, Reason} // in tests without a nil panic. func (e *RefRejectedError) Error() string { if e.err == nil { return fmt.Sprintf("ref %s rejected: %s", e.Ref, e.Reason) } return e.err.Error() } // Unwrap exposes the underlying receive-pack error so existing // errors.As(*packp.CommandStatusErr) checks — and substring inspection of the // leaseFailureMarkers: "non-fast-forward" / "fetch first" are excluded because an // update that is legitimately non-fast-forward and wasn't force-pushed looks // identical to a race, and treating it as a benign move would mask a real // "needs --force" failure. "remote ref has changed" is entire-server's // compare-and-swap rejection (storage.ErrReferenceHasChanged); "stale info" is // git's --force-with-lease lease miss. Both mean the server's actual tip differed // from the expected-old value this run sent. Match is case-insensitive substring // (Reason is free-form; see RefRejectedError). // "needs --force" failure.

// This set is server-specific by design. "remote ref has changed" is // entire-server's compare-and-swap rejection (storage.ErrReferenceHasChanged) — // the one git-sync's own targets emit, and the case that matters in practice. // "stale info" is git's force-with-lease lease-miss phrasing, kept for // consistency with leaseFailureMarkers and defence-in-depth; note it is // primarily a client-side status, so it may not arrive as a server ng reason on // every target. Stock git servers phrase a CAS miss differently again // ("failed to update ref" / "cannot lock ref ... but expected ..."), which this // set does NOT match — so against a non-entire target a genuine race may fall // through to a plain rejection. Extend this set as new server phrasings are // observed. Match is case-insensitive substring (Reason is free-form; see // RefRejectedError). var concurrentMoveMarkers = []string{ "remote ref has changed", "stale info", }