Timeline

* fix: clone_seqno is server-to-client only (#74)

The clone client transmitted a `clone_seqno` card to carry its pagination cursor. That card travels the other way: canonical's page_xfer() only ever writes it (src/xfer.c:1571) and has no parser for it, so a real fossil server answered `bad command: clone_seqno N` and the clone died on round two. Repositories that fit in a single batch never reached the pagination path, which is why wapp and pikchr cloned while fossil and sqlite did not.

The request cursor belongs on the clone card itself — `clone VERSION SEQNO`, read from token 2 at src/xfer.c:1553 — and the response cursor belongs on `clone_seqno`, which the client only consumes. Give each direction a single owner:

- The client sends its cursor on the clone card and never emits a clone_seqno card. It already consumed the server's card correctly; it now also rejects a negative value, matching src/xfer.c:2769. - The server reads its cursor from the clone card and the CloneSeqNoCard parser is gone, so there is exactly one way to set the cursor rather than two.

Aligning with canonical also corrects the cursor's meaning. Ours was the last rid sent, queried with `rid > cursor`; canonical's is the rid of the next blob to send, looped inclusively with `while( seqno<=max )`. Now that the value crosses to a real server it has to mean what canonical means, so emitCloneBatch queries `rid >= cursor` and reports the first rid it did not send, or 0 once the snapshot is exhausted. That collapses the old more/lastSentRID pair into a single nextRID and keeps the truncate hook honest by re-owing the dropped rid instead of skipping it.

TestCloneMultiBatchAgainstCanonicalServer drives the client against a stand-in server that implements the canonical contract and rejects a client-sent clone_seqno the way fossil does; it reproduces `bad command: clone_seqno` against the old client. TestHandleCloneCursorFromCloneCard pins the server half, including the inclusive cursor.

Verified against fossil 2.28 over a loopback server: fossil and sqlite now transfer every artifact (67,499 and 130,994 blobs), and wapp and pikchr still clone in 3 rounds.

* fix: conform clone cursor validation to the spec in both directions (#74)

Review caught two rules applied to the wrong side of the wire — the same axis error the original bug was.

The client rejected a negative clone_seqno as fatal, citing src/xfer.c:2769. That line is the server's rule for the SEQNO on a clone REQUEST. The reply parser is governed by §3.2 and §8.2: NEXT is recorded only when it is decimal, and a visible but non-decimal NEXT is a defined receiver-tolerance exception that leaves the recorded sequence unchanged and does not stop parsing later reply cards. Aborting the clone was the one thing the spec rules out. The rule belongs in the decoder, where a non-decimal NEXT now decodes to an unknown card: the token is preserved, no consumer records it, and following cards still parse. strconv.Atoi accepted "-1" off the wire, so this was reachable, not theoretical.

Conversely the server clamped `clone 3 0` to a cursor of 1 where §8.1 mandates a fatal: clear accumulated output, roll back, emit `error invalid clone sequence number`, and parse no later request card. Nothing is written before processDataCards, so returning early is what the rollback amounts to. Seeing this case at all needs a presence flag, because `clone` and `clone 3 0` both parsed to SeqNo 0 and only the former is exempt. §8.1 also keys on a digit-only SEQNO and withholds the fatal from wrong arity, a VERSION below 2, and a SEQNO that fails digit-only recognition, so the flag records digit-only presence rather than merely arity.

Both card rules now turn on the spec's `decimal = 1*DIGIT` production via a shared predicate instead of on whatever strconv.Atoi tolerates.

Also promotes emitCloneBatch's `count > 1` truncate guard to a documented liveness constraint. Under the old exclusive `rid > cursor` semantics `count > 0` was safe; under the inclusive cursor, dropping the only card in a batch reports a cursor equal to the one we entered with and the client re-requests forever. A check before the reply card now names a stalled round at its source instead of letting it surface as an opaque MaxRounds failure, and the cursor precondition became an error return rather than a panic — it is enforced ~350 lines away in a network-facing handler.

Records the deliberate asymmetry that our server ignores an inbound clone_seqno where canonical answers `bad command`, and renames TestHandleCloneSeqNo, which no longer sends that card.

* fix: name the clone SEQNO flag for the shape it records (#74)

HasSeqNo described presence, but the field records whether the SEQNO token is digit-only: `clone 3 -1` carries a SEQNO and sets it false. §8.1 keys its fatal on that shape rather than on the parsed value, which is why a plain int cannot stand in for it. SeqNoIsDecimal says so.

Reading it as presence would also collapse `clone 0 0` — a distinct wire form that parses to the same Version and SeqNo as a bare `clone` — back into `clone` on re-encode. encodeClone already guarded against that; a round-trip test over every clone wire form now pins it, and fails on exactly that input when the guard is removed.

Also records why the §8.1 rollback is currently vacuous: no card handled before processDataCards writes anything, so returning early leaves nothing to undo. Nothing enforces that ordering, and §3.5's ci-lock would break it, so the dependency is written down next to the code that relies on it.

Corrects a test comment that claimed a continuation property it did not exercise — the malformed card sits in the trailer, so nothing follows it. That half of §8.2 is covered by the follow-on card in TestDecode_CloneSeqNoNonDecimal. check-in: f2fa2cc751 user: [email protected] tags: trunk

* Deltify predecessors on the commit path

Every locally-created artifact was stored whole. The delta table was in the schema and stayed empty, so a repository grew with the number of commits rather than the amount of change. blob.StoreDelta existed and worked but had no caller on the commit path; the only delta-store in use was blob.StoreDeltaRaw, from the sync receive path, for deltas that arrive pre-encoded. The write path never made a deltify decision at all.

Port canonical Fossil's content_deltify (2.28 src/content.c:852-947) as content.Deltify. Fossil deltifies backwards: a new artifact is always stored whole and its PREDECESSOR is rewritten as a delta against it, so the tip of every chain is full content and reading the newest version of a file stays a single blob load. Chain depth becomes an artifact's age in revisions rather than something that compounds, because an artifact already stored as a delta is never re-deltified.

blob.StoreDelta turned out to be the wrong primitive to wire up: it inserts a new row as a delta, whereas the policy needs to rewrite an existing row. It is left alone. content.Deltify lives in the content package because it needs Expand, which blob cannot import.

The policy is stated once, in content.Deltify, with each rule's canonical line cited: the 50-byte floors, the 75% ratio, the never-redeltify rule that bounds depth, the private-into-public refusal, and the loop break that undeltas the source and then still declines the pairing. Two call sites pass pairs, mirroring canonical's: insertMlinkRow for file content (manifest.c:1562) and Checkin for the parent manifest (checkin.c:3234). insertMlinkRow is shared by the fresh-commit and crosslink paths, so file deltification cannot drift between them.

On a 12-revision, 4-file history: 60 artifacts, 44 delta-encoded (73%), p50 chain depth 4, max 11, and 23,994 content bytes measured as sum(length(content)) -- down from 58,973 with everything whole. A repository produced this way passes fossil 2.28 test-integrity (60/60 blobs, 0 errors) and fossil rebuild, which re-derives every table by walking the chains.

* Bound the deltify ancestor walk, require a transaction, harden tests

Review follow-ups on the deltification policy.

Bound the ancestor walk. It was a bare loop with no cycle guard: a sync peer can shape a cyclic delta graph, because blob.StoreDeltaRaw records delta(rid,srcid) links from the wire without a cycle check, and an unbounded walk would spin inside the write transaction. Guard with the seen-set idiom content.walkDeltaChain already uses, plus a step cap. In the current ordering IsAvailable runs first on both rids and follows the same links under its own depth bound, so Deltify declines before the walk begins -- but that protection is incidental to a different question, so the bound belongs to the loop that needs it. Both behaviours are now pinned: the walk reports a cycle, and Deltify declines an ungrounded source.

Move both size floors ahead of the walk. Canonical returns on a sub-50-byte target (content.c:877-885) before entering the candidate loop, so it never reaches the content_undelta on :902. Running the walk first meant a small target whose successor chained back to it would inflate the source to full content and then decline anyway -- a storage regression and exactly the divergence from canonical's layout the surrounding comment claims to avoid.

Take *db.Tx instead of db.Querier. The rewrite is two statements, and a blob holding delta bytes with no delta row expands as though it were full content: silent corruption rather than a detectable error. Canonical wraps the pair in db_begin_transaction (content.c:938-941); the signature now makes that non-negotiable at compile time. Both call sites already passed a transaction.

Three tests were named for behaviours they did not pin, and each is now verified to fail when the behaviour is reverted:

- The 75% ratio test had no assertion at all; deleting the rule left it passing. It now asserts the pair is declined and stays whole. - The loop-break test could not distinguish declining from proceeding, so the undelta-then-proceed behaviour corrected in the previous commit was untested. It now asserts the target is left whole. - The manifest test checked only "at least one", so a regression from 11-of-12 to 3-of-12 passed. It now checks each index and names the manifest that stopped being encoded.

Also cover the source-side 50-byte floor, which the tiny-artifact test could not reach because it made both artifacts tiny and returned at the target check first, and assert that Undelta restores blob.size while Deltify leaves it alone (content.c:761 vs :935).

Gate the fossil-binary test on REQUIRE_FOSSIL_BIN, set in CI. A skip is invisible without -v, so a run with no fossil binary was byte-identical to a pass -- for the one criterion our own tests cannot substitute for.

Document that blob.StoreDelta is not the commit path's primitive. It inserts a new row as a delta; deltifying rewrites an existing one. It has no non-test callers and the package doc advertised it as the delta-storing entry point.

* Document what the deltify loop-safety argument rests on

Two comments, no behaviour change.

The IsAvailable(srcRid) guard is ours, not canonical's: content.c guards only content_is_available(rid) and then calls content_get(srcid) unguarded. It is also load-bearing beyond phantoms, because it screens out the cyclic delta graphs that would otherwise reach the ancestor walk. Say both, at the guard.

And say at the walk why its seen-set never fires in practice, so that someone reordering the prologue meets the reason rather than concluding the bound is dead code. The note lived only in the test file, which is not where that reader is looking. check-in: 2368cad91a user: [email protected] tags: trunk