Address re-review: settle unconfirmed creates against the target, not its prose

main

Commit

nodo4d ago

The previous round inferred "the branch is there" from the ng text the target happened to write, using gitproto's concurrent-move marker set — a substring heuristic built to classify compare-and-swap misses, and documented as server-specific. A pre-receive message like "refusing to create refs/heads/main: a tag with that name already exists" matched it, deleted the resume marker with the branch still absent, and reintroduced the exact re-transfer this PR exists to prevent. It also had no answer for a target that never advertises report-status: markers were kept on runs that fully succeeded, permanently, since the bootstrap route forbids --prune.

Both are the same mistake — guessing at target state — and there is one honest answer: ask. When a push leaves a create in doubt, the marker is held back rather than decided, and after the last branch a single ref listing settles every doubtful case at once. Branch present, whoever created it: the marker is stale scaffolding and goes. Branch absent: it is a resume position and stays, with the reason stated accurately, which the old notice did not — it claimed "did not land" for creates that had landed and promised a resume that could not happen. Listing unavailable: everything stays, and the run still succeeds, because a listing failure at the end of a multi-gigabyte import must not fail an import that was delivered. The common path pays nothing: a confirmed create still deletes its marker immediately, with no extra round trip, pinned by a test. gitproto.IsConcurrentMove goes back to being unexported.

Also from the review, all confirmed by reading the code:

  • The ng reason reached the terminal unsanitized. gitproto stores the raw status and only OnRejection filtered it, so the notice printed server text verbatim — an escape sequence could clear the warning's line and redraw it as a success, which is the attack internal/sanitize exists for. Filtered where s.rejections filters, with a test that fails without it.
  • A refused delete of a stale marker returned nil and the run restarted from zero, pushing the next checkpoint with Old=zero against a ref still on the target: a CAS failure every subsequent run reproduces, needing a hand cleanup. Now stops, naming both the refusal and why the marker was stale.
  • A refused delete of a finished marker was silent. Now logged and surfaced, non-fatally, since the branch is there.
  • A report that omits a command git-sync sent left that ref looking unrejected. gitproto now treats an unauthored silence as a rejection rather than as success.
  • The subsumed path uses the refusal directly; there is no marker to settle and nothing to clean up either way.

Fixture, also from the review: refCreateDenier's off/denials were shared with the httptest goroutine unsynchronized (latent under CI's -race); the no-report-status flag was honored only on the packful path, so delete-only pushes silently got a reporting target; the unpack and apply-commands blocks existed in three copies that had already drifted apart on compare-and-swap fidelity — now one helper each, so every path enforces Old; and assertCommitHistoryPresent claimed to check blobs while only resolving trees. It now walks trees and blobs and compares the target's commit count with the source's, so "the import landed" is checked rather than asserted.

Left open, and not this PR's: the one-shot path still justifies never deleting its marker by deferring to prune, which Bootstrap() forbids — the same permanent-marker gap, one function up, from #116. Now cheap to close with the same listing.

Full suite green under -race, golangci-lint clean.

🤖 Generated with Claude Code

Co-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com Claude-Session: https://claude.ai/code/session_018BKBGBh3rJ6WGnZKDbBRy7 Entire-Checkpoint: 01M1BYCREMPSP93AWNJ6YNDWAS

Checkpoints

Fix Batched Resume Marker on Branch Create Refusal

Claude CodeOpus 5
View session
Checkpoint 1