Timeline

* fix: correct go.mod module path to go-libfossil (#76)

The module directive still declared github.com/danmestas/libfossil after the repository was renamed to go-libfossil. pkg.go.dev/godoc scraping matches the declared module path against its hosting path, so the stale path broke SDK doc generation.

Rename the module path across all four in-repo modules (root, the two db/driver submodules, observer/otel, and docs/site) and every internal import, plus the release workflow, the doc-generation script, and the docs.

No tag has been published under the go-libfossil path yet, so the cross-module requires cannot resolve from the proxy. Add replace directives pointing the in-repo requires at their local directories so GOWORK=off builds and the CI matrix resolve without a published version. These replaces are ignored by downstream consumers and can be dropped once new-path tags are cut.

* Guard releases against replace directives; regenerate SDK docs

The go-libfossil rename bridges the in-repo modules with `replace` directives. Go refuses a main module that carries them, so a tag cut while they are present breaks `go install .../cmd/libfossil@<tag>` and every `@version` fetch of the driver submodules. Add a Release workflow step that fails the job before anything is published if any tracked go.mod still contains a replace directive, so the bridge is a CI-enforced TODO rather than a silent landmine. The step comment records what unblocks a release: re-tag the submodules with the corrected module paths, bump the root requires, then drop the replace blocks.

Regenerate the SDK reference docs. The previous pass only substituted URLs over the pre-existing generated files, leaving stale signatures (Repo.Timeline was still documented as taking LogOpts rather than TimelineOpts). Note that gomarkdoc must be run outside a git worktree — inside one it silently strips every source link.

Also reword the README rename note, which read as though the import path had not changed, and re-sort the ncruces require block. check-in: 6273a0750d user: [email protected] tags: trunk

deltifyBreaksLoop bounded its ancestor walk on deltifyMaxChainWalk (1,048,576), 64x the maxDeltaChainDepth (16,384) cap the read-path walks use, with no rationale for the difference. It is the same delta table over the same chains, so the smaller bound's reasoning applies verbatim.

Bound deltifyBreaksLoop on maxDeltaChainDepth and remove the now-unused deltifyMaxChainWalk constant. Behaviour is unchanged for any real corpus -- the bound only bites far beyond the deepest observed chain (2,546). check-in: 17724bfdfd user: [email protected] tags: trunk

* fix: never parse a compressed body as cards (#104)

A clone served by libfossil failed in libfossil's own client with

xfer: decode card 16303: xfer: empty line after split

on a response that contained 202 cards. The index, the card, and the fault were all fictional.

xfer.Decode tried three body framings in order -- raw zlib, the §4.1 4-byte-length container, then uncompressed card text -- and treated any decompression failure as "this is not that framing". decompressBounded failed a well-formed container purely because its decompressed length passed a 50 MB bound, so the fallback chain handed the still-compressed bytes to the card parser. The parser read compressed noise as cards for sixteen thousand iterations before one of them split into no fields, and reported that as the fault. Captured at offset 15,084,752 of the failing body, the "card" the parser was reading is DEFLATE output:

"...\xd9\x34\x50\xe7\xcf\xa0\xf8\xd5\xd5\x87l\xff\t\xde\xfb\xe3..."

Two changes. Once a body is recognized as a zlib stream, a failure to decode it is reported rather than reinterpreted -- oversize, truncated, or corrupt, the error now names the transport failure instead of an arbitrary card position. Both compressed framings are still attempted, since a length prefix can coincidentally parse as a zlib header, and uncompressed bodies still reach the card parser; only compressed ones no longer can.

The bound itself was also wrong, and is the reason this was reachable at all. §4.1 places no limit on the decompressed size of the container; the 50 MB was a local memory guard that sat below what this implementation's own server emits in one clone round, so two libfossil peers could not clone from each other even though each could clone from fossil. It is now 64 MiB and coupled to sync.DefaultCloneBatchBytes by a compile-time guard that fails the build, rather than the clone, if the budget is raised past half of it.

The defect predates the size-bounded clone batching of #100. #100 shrank rounds enough to hide it on the corpus that exposed it; it did not touch the decoder.

Regression coverage, in two layers. internal/xfer pins the decode behaviour directly: an oversize container, a truncated one, a large in-bounds one, and an uncompressed body. internal/sync adds the configuration no existing test covered -- libfossil serving and libfossil cloning across a real HTTP encode/decode, on a corpus of multi-megabyte incompressible artifacts that crosses the server's per-round budget. Every prior clone test was either served by fossil or wired through MockTransport, which passes *xfer.Message values across untouched and never encodes a wire body at all. Artifact size is the other half of the reproduction: this failed on wapp, whose artifacts are large, and not on pikchr, whose artifacts are small.

Verified against the wapp corpus: the round trip that failed now completes with 1038 of 1038 artifacts on both ends.

* fix: make the #104 fixture reach the bound, and pin the bound from both sides

Review found the flagship regression test passed against pre-fix code. It did. Its corpus was 21.6 MB spread over twelve artifacts, and pagination keeps every round near the 16 MB budget however large such a corpus grows, so the round never came near the 50 MiB bound it was supposed to cross. The 25-line rationale reasoned about crossing DefaultCloneBatchBytes, which is not the quantity that matters and never was; a permanent comment asserting a mechanism that does not exist is worse than no comment.

The quantity that matters is one artifact. A round is charged against the budget before each artifact and sends whole the artifact that crosses it, so a round carries budget plus one entire artifact of unbounded size. A single artifact larger than bound-minus-budget is the only corpus that can exceed the bound, and it is also the one path by which a round can still exceed the current bound -- so the fixture guards the live risk and reproduces the historical one with the same bytes. It is now one incompressible artifact of 7/8ths of MaxDecompressedBytes, sized against the constant so it tracks it, behind testing.Short.

Verified against a pre-fix tree with both test files dropped in:

TestCloneSelfRoundTripOversizeArtifact FAIL Clone: handleFile aad935c2...: UUID mismatch for received file: expected aad935c2..., got 501b4814...

which is the sharper end of this defect: pre-fix did not merely misreport, it accepted the compressed bytes as cards and produced an artifact whose content did not hash to its name.

The cheap end-to-end round trip is kept as a separate test and described honestly as a coverage plank -- the first test anywhere here to have libfossil decode a body libfossil produced -- rather than as a guard on anything.

Second finding, message.go: the claim that format 1 is the "Fossil HTTP sync wire format" is false. Every application/x-fossil body carries the §4.1 length prefix, request side and reply side, both through blob_compress. Format 1 matches nothing canonical emits and is kept as a tolerance, now labelled as one.

Its safety was also load-bearing on a constant with no stated relationship to it. For a container of decompressed length N, prefix byte 0 is N>>24, and a zlib CMF requires CM==8 in the low nibble, so no container under 128 MiB can present a valid header at offset 0. Raising MaxDecompressedBytes past that point would leave decompressContainer's prefer-what-decodes ordering as the only remaining defense -- a narrower margin than this code was written against, and #98 is exactly the change that might want a different bound. That is now a build failure rather than a trap, matching the guard already on the other side. Verified to fire at 0x08000000 and to permit 0x07FFFFFF.

Also: the MaxDecompressedBytes doc claimed to stop a hostile body from exhausting an embedding process's memory. It does not -- the transport reads the body whole before Decode sees it. It bounds expansion only, and now says so.

Test hygiene: the server handshake polls for a live listener instead of sleeping 200ms and dropping ServeHTTP's error, and the 32 MiB and 64 MiB xfer allocations skip under -short like their sync counterpart.

* fix: correct three claims about what the #104 guards cover

The fix is unchanged. Three statements about it were wrong, and a comment that misstates a mechanism is worse than no comment because the next reader trusts it.

The ordering claim was the serious one. decompressContainer was described as "preferring whichever framing actually decodes, so a spurious header match still loses to a real container". It does not. Format 1 returns immediately on success and format 2 is reached only when format 1 fails; the error comparison at the end selects which failure to report when both have already failed, and protects nothing when format 1 spuriously succeeds. Probed: a bare zlib body returns from format 1 with format 2 never attempted.

That makes the aliasing guard weaker than its comment claimed, and in a direction neither review had reached: the aliasing byte is the top byte of the length the PEER declared, and this decoder never reads that field -- it slices past it and counts bytes actually inflated. No value of MaxDecompressedBytes constrains what a peer declares. A container declaring 0x081D0000 while carrying 46 bytes presents a parsing zlib header at offset 0, measured.

What actually saves a real container in that case is the stream, not arithmetic: after two prefix bytes are eaten as a header, inflate must still parse the remaining two plus the real stream as DEFLATE and match its Adler-32. Measured `flate: corrupt input` at all three aliasing lengths probed, so format 1 fails, format 2 runs, and the container decodes correctly.

The guard is kept because it states something true and useful -- Encode writes uint32(len(raw)) as the prefix, so holding the bound below the threshold means no body libfossil emits can carry an aliasing prefix -- but it is now documented as an invariant on our output, not a defence of our input, with the airtight alternatives named and left to their own issues.

Second, "a single artifact larger than this bound cannot be cloned" is true but not the operative threshold. A round already holding bytes has that much less room, so the ceiling is the bound minus what the round accumulated first. Measured on this branch with a 58,720,256-byte artifact: 8, 12 and 15 MB of filler ahead of it all fail, and 17 MB passes because it exhausts the budget and pushes the artifact into a round of its own. Non-monotonic in the filler, which is worth knowing. That is #109, a defect this fix exposes rather than causes; the fixture's claim to guard it is withdrawn and it now says plainly that it covers the leading-artifact case only.

Third, the changelog's "pin the bound from both sides" overclaimed. The guards pin the bound against the batch budget and against the aliasing threshold. Neither certifies a round.

Rebased onto 8b6da9a. The internal/content -race failure reported last round was not a flake and not unrelated -- this branch predated #107, which rewrote available.go. TestIsAvailable_CyclicChainTerminates goes from exceeding its 30s budget to 0.06s across that rebase.

---------

Co-authored-by: Dan Mestas <[email protected]> check-in: c892c7a622 user: [email protected] tags: trunk

* perf: memoize delta-chain interiors during crosslink (#89)

manifest.Crosslink ran at roughly 1.4 events/sec on the Fossil SCM repository, which puts a 27,000-artifact clone at two and a half hours of CPU after the transfer has already finished. A CPU profile of a sweep over that corpus attributes 152.77s of Crosslink's 167.20s -- 91% -- to content.Expand, and 126.35s of that to the single blob.Load inside its delta-replay loop.

The cause is chain depth, not per-event overhead: not a transaction per event, not a missing index, not a full-table scan. content_deltify stores a file's older versions as deltas against its newer ones, so one file's history is a single chain -- 469 nodes deep on average across that repository, 1,032 for the blobs carrying artifacts, 2,546 at worst. Expand walked each chain from its root on every call and Crosslink calls it once per blob, so the sweep performed 31.7 million delta applications to expand 67,451 blobs.

Cache.Expand now stops the chain walk at the deepest ancestor it already holds and keeps every node it materializes on the way forward, and Crosslink runs one such cache for the sweep. Blob content is immutable once written and the candidate rids are snapshotted before the loop, so nothing a sweep does can invalidate an entry.

Every node handed to the cache is verified against the UUID its blob row is stored under before it is stored, because a cached interior is served later as if it had been expanded in its own right. The number of hash checks per sweep does not change: each blob is materialized once and verified once, where before each was expanded once and verified once. A plain Expand, with no cache, walks and verifies exactly as it did.

IsAvailable folds its two-statements-per-chain-node walk into one. It runs once per F-card of every check-in being crosslinked, over the same deep chains, and every SQL round trip there pays a WAL read-lock acquisition -- the round trips, not the work they do, are the cost.

No transaction boundary moves and no batching is introduced. A manifest that fails to crosslink still fails on its own, exactly where it did.

Measured on the Fossil SCM repository, 67,451 content blobs and 27,060 artifacts. Time for Crosslink to write its first 300 event rows: 221s before, 131s after. Over the expansion path in isolation, sweeping the same corpus in rid order: 22.9 blobs/sec with no cache, 50.1 at the 256 MiB budget set here, 414 at 1 GiB -- where the process needs 2.9 GB resident, which is why 1 GiB is not the number.

A bounded cache cannot close the gap on its own. Chains run from a high-rid root down to low-rid ancestors while the sweep visits rids ascending, so the first candidate of a chain materializes all of it and the remaining nodes are consumed arbitrarily far in the future; the working set of a whole-repository sweep in rid order is the whole repository expanded, 8 GiB for this corpus. Closing it means visiting candidates in delta-chain order, which is how Fossil's own rebuild avoids the cost, and which changes the order-sensitive state Crosslink writes -- leaf, tag propagation, tech-note event replacement, forum thread titles. That is a separate change.

TestFossilBinaryReadsCrosslinkedRepo covers the acceptance criterion this cannot self-certify: a repository whose relational tables were written through the accelerated path passes fossil 2.28's test-integrity, shows its tip in fossil timeline, survives fossil rebuild, and its event, plink, leaf and mlink rows match, column for column, what fossil derives from the same blobs.

* fix: bound and cycle-check the delta-chain walks (#89)

IsAvailable walked the delta chain with no cycle detection, to a bound of 100,000 against real chains whose deepest observed node count is 2,546. It is wire-reachable -- sync/clone.go, sync/handler.go, and once per F-card of every check-in Crosslink processes -- so a peer that plants a two-node delta cycle bought a 100,000-iteration query loop on every call, inside whatever process embeds this library.

Both halves are fixed. Each walk now carries a visited set and stops at the first repeat, which is the actual defence: a cycle costs its own length, not the bound. maxDeltaChainDepth replaces the old constant at 16,384, roughly six times the deepest chain measured on the Fossil SCM repository, and it is now a backstop for an acyclic-but-absurd chain rather than a number an attacker spends. walkDeltaChain, which the memoizing expansion path made hotter, previously had no bound at all and now shares the same one.

This is what made TestIsAvailable_CyclicChainTerminates sit at ~99% of its 30s budget on main and fail outright under -race here: the cyclic walk was running the full bound. It now finishes in about a second under -race, six runs out of six.

walkDeltaChain returns the content it stopped on instead of leaving expandChain to ask the store a second time. The truncation decision and the use of it were separate steps, so a concurrent eviction between them would send a delta row to blob.Load and hand raw delta bytes to the replay as full text. bindToName caught that and failed closed, but the caller saw a hash mismatch on an artifact that was not corrupt, and Crosslink's error path drops those silently. Unreachable while the cache is a single-goroutine local, but Cache is documented concurrency-safe and takes its mutex on every method, so the type promised something the expansion path did not deliver.

Cache entries are clipped to their length before being stored, so curSize is the bytes actually retained rather than an allocator-dependent approximation of them.

Test corrections: the corpus harness no longer closes the repository handle out from under the sweep goroutine when it returns at the event target; the sweep owns the handle and closes it. snapshotDerived now fails on an empty digest rather than comparing "" to "" and passing without examining a row. The fail-closed injector matches the merged chain-step statement specifically rather than any query mentioning delta, and says what it can and cannot prove now that the two lookups are one statement. The fossilbin comparison documents that it is not full-table equivalence, and which tables it leaves uncompared.

MEASUREMENTS, SUPERSEDING THOSE IN 2426591

The figures in the parent commit were taken before clipping, and its "221s -> 131s" pair predates the double-hash and IsAvailable fixes that landed in that same commit. Everything below was re-measured on the code as it ships, one run at a time on an idle machine, against the Fossil SCM repository (67,451 content blobs, 27,060 artifacts).

Crosslink to its first 300 event rows, cpuprofile on throughout:

original code 221.4s as shipped 103.9s 2.1x

Expanding the first 4,000 blobs, which isolates the expansion path:

budget throughput peak RSS entries held none 22.1 blobs/s 169 MB 0 0 256 MiB 52.0 blobs/s 1036 MB 543 255 MiB 1 GiB 468.3 blobs/s 2278 MB 15,842 1021 MiB

Clipping changed neither throughput nor memory. Crosslink to 300 events runs 103.9s clipped against 105.9s unclipped, peak RSS 1067 MB against 1050 MB, and the cache holds the identical 543 entries and 255 MiB at a 256 MiB budget either way -- at these sizes the buffers already arrive tightly sized. It is worth keeping as an accounting guarantee, not as a saving, and the comment on clip says so.

The earlier claim that capacity slack was why a 256 MiB cache reached nearly a gigabyte resident was wrong. Peak RSS tracks the no-cache baseline plus roughly twice the budget, because the cache is live data and the collector sizes the heap against it: 169 + 2x1021 = 2211 against 2278 observed at 1 GiB. That is why the budget stays at 256 MiB -- a gigabyte of cache costs over two gigabytes resident to clone a 72 MB repository. The shape of the curve did not move.

* docs: correct the claims around the chain bound and the RSS model (#89)

Three claims did not survive checking, and one bound was a node out.

maxDeltaChainDepth said it bounds "every delta-chain walk in this package". It does not: deltifyBreaksLoop is a third walk, under deltifyMaxChainWalk, which is 64 times larger. The rationale on the new constant applies to that walk just as well, but changing it would change deltify's shipped behaviour, so the sentence is narrowed to the two read-path walks it actually governs and points at the third rather than quietly covering it.

Two comments still named maxAvailabilityChainDepth, which no longer exists, and argued from IsAvailable having a bound "of its own" -- the framing of a world where the two walks had separate bounds. Against a shared constant that argument reads backwards. Both now name the shared bound, and the reason deltifyBreaksLoop keeps its own visited set and its own step cap is stated as the general rule it is: every walk over the peer-shaped delta graph carries both, and none of them relies on another walk's guard.

The retraction in the previous commit replaced one unsupported mechanism with another. "Peak RSS runs at roughly the baseline plus twice the budget" is a two-point fit whose worse point is the shipped one: it is 3% off at 1 GiB and 53% under at 256 MiB, where resident memory is four times the budget rather than twice. The measured table stays; the model is demoted to the direction the data supports, that peak RSS grows faster than the budget, with the fit's own weakness stated.

walkDeltaChain counted loop iterations where IsAvailable counts nodes visited, so the shared bound bit a node apart: a chain of exactly maxDeltaChainDepth+1 was unavailable to one walk and complete to the other. It now counts nodes, and TestChainBoundBitesAtSameLength pins the agreement at both boundaries -- nothing else ties the two loops together. check-in: 9864120a32 user: [email protected] tags: trunk

* fix: bound clone batches by output size, not artifact count (#88, #92)

The xfer server paginated a clone by a fixed 200-artifact count. A count cannot adapt to artifact size: 200 control artifacts and 200 large blobs produce wildly different messages, and neither corresponds to what the transport can carry. It capped a libfossil-served clone near 20,000 artifacts against our own MaxRounds=100 client, and left the response size unbounded in the other direction -- serving libfossil's own repository produced a single 44.9 MB round.

emitCloneBatch now accumulates the wire bytes it emits and stops once they reach DefaultCloneBatchBytes, mirroring canonical's `while( mxSend > blob_size(pOut) )`: the budget is tested before each artifact and the artifact that crosses it is still sent whole, which is what keeps the cursor advancing when one artifact exceeds the entire budget. The card header is charged alongside the payload so a repository of zero-length artifacts still paginates. Section 8.2 leaves the bound to the server ("clients MUST NOT infer count or size from range").

The budget is 16 MB rather than canonical's 5,000,000 max-download because this clone path emits fully expanded artifacts where canonical retransmits stored compressed form (Section 7.2) -- 80x to 220x the bytes on real repositories. Measured on libfossil's own repository (17,218 artifacts): 87 rounds and a 44.9 MB worst-case response before, 65 rounds and 24.7 MB after. Canonical's constant applied to expanded bytes would have needed 184 rounds, breaking clones that work today.

Adds the interop half: a canonical fossil 2.28 client cloning a generated corpus from `repo serve`, sized past the pagination boundary, checked for round-trip count, artifact parity with the source, and `fossil test-integrity`. The many-small-artifacts case is what discriminates the two bounds -- 2,056 artifacts clone in 3 round-trips under the size bound and 23 under the count bound. Corpus size is a parameter since the interesting behaviour is at the boundary. A missing fossil binary skips, or fails under REQUIRE_FOSSIL_BIN=1.

* review: state the ceiling honestly, fix the overhead comment

The size bound replaces an artifact-count ceiling with an expanded-bytes ceiling. It is a better ceiling and it beats the count bound on every corpus measured, but fossil- and sqlite-sized repositories still cannot be cloned from a libfossil server within MaxRounds. Record the full three-repository round table on DefaultCloneBatchBytes and say outright that only Section 7.2 removes the ceiling rather than moving it.

State the memory property as budget plus the largest artifact, not as a 24.7 MB ceiling. The crossing artifact is sent whole, so a repository holding one 2 GB artifact still emits a 2 GB round, and concurrent clones multiply it. 45 MB -> 25 MB is a large reduction of a pre-existing unbounded allocation, not a cap.

cloneCardOverheadBytes claimed to cover the hash and then len(uuid) was added on top, double-charging it. Drop the addend; 96 over-approximates the ~58-byte framing on its own and the slack absorbs the uncharged PrivateCard.

Also: smallBatch sets a one-byte budget rather than a batch size of one, so correct the comments that still describe the old knob; drop the post-loop multi-batch > single-batch check, which passes under both bounds and so discriminates nothing; and make serveRepo report a server that exited early instead of signalling into an uninstalled handler, where SIGINT's default action would kill the test binary outright.

* test: count clone batches on the server, not round-trips on the client

CI (fossil 2.23) and this machine (2.28) disagreed on an identical server: the multi-batch clone reported 1 round-trip against CI and 9 locally. The clone was correct in both cases.

fossil prints "Round-trips: N" carriage-return separated, once per round. 2.28 guards those prints behind bOutIsTty and adds a single final line for non-TTY output, so a captured clone yields exactly one occurrence holding the total. 2.23 has no such guard and emits every round into the capture, so the regex's first match was always 1 no matter how many rounds ran. Verified by cloning one repository from a real fossil server with both binaries: 2.28 prints "Round-trips:" once, 2.23 four times, first line 1 and last line 2.

So the assertion silently meant different things on different clients, and many-small-artifacts -- the only case that discriminates a size bound from a count bound -- was passing under 2.23 for the wrong reason, inert rather than green.

Count the clone batches the server emits instead. That is the property under test, it cannot be distorted by client-side reporting, and it is sharper: many-small-artifacts is 2 batches under the size bound against 22 under a reinstated count bound, where the round-trip numbers were 3 against 23.

This serves via sync.ServeHTTP directly so the handler can be wrapped. RepoServeCmd.Run is a five-line wrapper around the same call and TestRepoServeCmdCanonicalFossilClone still drives the command end to end. It also drops these subtests' use of the process-wide SIGINT. check-in: 37271d3f68 user: [email protected] tags: trunk

* Add libfossil version command

Sources the version string from runtime/debug.ReadBuildInfo (module version for go-install/tagged builds, falling back to a short VCS commit), with a -ldflags override for release builds where BuildInfo alone would collapse to "(devel)". Output is a fixed, single-line, four-field shape ("libfossil <version> <go version> <os>/<arch>") so a caller can rely on strings.Fields(Version()) unconditionally -- the motivating consumer stamps this into every emitted benchmark record.

No global --version flag: repo extract already owns --version as a command-scoped flag (source version to extract), and kong's global flags are visible in every subcommand's context, so a root-level --version would collide with it. version is a subcommand only.

Verified kong's existing parse-error path already exits non-zero (exitUsageError, 80) for unrecognized commands and flags against the pinned kong version -- added regression tests locking that in rather than changing behavior that was already correct.

* Reject malformed -ldflags version overrides at startup

buildVersion was used verbatim with no validation, so a whitespace- or newline-containing -X override silently broke Version()'s promised single-line, four-field contract: a space produced six fields instead of four, and a newline broke the single-line guarantee outright. The stated consumer is a benchmark harness parsing this line as a stable record, so a corrupted version field silently corrupts every record from that build in exactly the field meant to identify it.

A bad -X value is a build-time operator mistake, not a runtime condition, so it's rejected at package init rather than sanitized or worked around: sanitizing would silently mangle the operator's intent, and falling back to build info would report a version that is confidently wrong. Both would hide the mistake in the one field whose job is to be trustworthy. Failing the whole binary at startup surfaces a bad build the first time it runs, for any command, not just when someone happens to run `version`.

Also simplified versionFromBuildInfo: verified empirically (go1.26, `go build .` inside this checkout, with and without -buildvcs and inside/outside a VCS checkout) that Main.Version is only ever the uninformative "(devel)" in exactly the cases where vcs.revision is also absent from BuildInfo.Settings -- whenever a VCS revision is available, Go has already folded it into Main.Version as a pseudo-version. The separate vcs.revision fallback loop was dead code that could only ever duplicate a hash already present in Main.Version; removed it along with the now-unused shortCommit helper. check-in: 725c694cb9 user: [email protected] tags: trunk

* fix: remove StatusOpts, MergeOpts, CheckoutOpts — no call sites

No function in the package accepts any of these three types, and nothing constructs one anywhere: Checkout.Status() takes zero arguments, Repo.Merge takes positional string arguments, and CheckoutOpts had no construction site at all. A public type with no call site documents a capability that does not exist -- a consumer reading MergeOpts{Strategy: ...} in the docs could reasonably conclude a strategy-selecting merge API exists; it never did.

Repo.Merge and Checkout.Status keep their current signatures. An options-struct refactor for Merge was considered and declined.

CheckoutOpts.Force was also a third same-named Force field in this package, alongside the already-removed UpdateOpts.Force (#58) and the real, fully-wired ExtractOpts.Force -- removing it also removes that readability hazard. ExtractOpts.Force and its plumbing (checkout.go Extract -> internal/checkout.ExtractOpts -> extract.go's Force check) are untouched.

* docs: match em dash style in CHANGELOG breaking-change bullet

The new StatusOpts/MergeOpts/CheckoutOpts entry used ASCII double hyphens where the adjacent CloneOpts and UpdateOpts.Force bullets in the same file use a real em dash. check-in: 1a33f22119 user: [email protected] tags: trunk

* 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

* Enforce card occurrence classes and intra-run ordering

The deck parser enforced card-type ordering and nothing else. Each of the 14 single-occurrence card types was handled by a case that assigned, so a second occurrence silently overwrote the first, and the repeatable types were appended with no sort check at all. draft-fossil-artifact-format-00 §4.5.1 and §4.5.2 define both rules; implement them.

Duplicate rejection covers A, B, C, D, E, G, H, I, K, L, N, R, U and W. The guard is separate from the card-type check because that check permits equal consecutive letters. F, J and M gain a strictly-ascending check against the preceding entry, T the two-level (name, target hash) rule with the full sign-prefixed name as primary key. P and Q deliberately get neither: P order is semantic — primary parent first — and Q is unordered-repeatable. P also now appends across cards instead of replacing, which is what §4.5.1 means by repeatable. Z belongs to neither class, so it is accepted with no duplicate or last-card rule; the authoritative checksum is the trailing 35 bytes VerifyZ already verified.

All four ordering checks route through one comparator, deck.Compare, which is §4.5.3: unsigned octets, locale-independent, no normalization, no case folding. §4.5.3 exists because the same ordering underlies the delta-manifest merge walk and baseline binary search of §8.2, so a second copy would resolve a delta manifest wrongly rather than merely fail validation. The F check runs on the decoded filename, per §4.5.2.

Malformed input is reported through the error return, never a panic -- these bytes arrive from peers.

Two writer-side defects surfaced, both of which let this package emit artifacts its own parser now rejects. Marshal emitted J cards in caller-supplied order while already sorting F, M and T; it now sorts them. Marshal also sorted T by the concatenation name+target, which lets the target's leading hex digit compete with the tail of a longer name and reorders tags whose names are prefixes of one another; it now uses the two-level key. The F and M sorts and the R-card file enumeration route through deck.Compare as well, the latter per §6.3.

Closes #70

* Order T cards by decoded name and type the Z payload

Review found two ways the first pass diverged from canonical fossil in the accept/reject direction.

The T ordering key used the raw wire name. §4.7.16 says the name is escape-decoded and that the primary ordering key is the full name token including its leading sign character; manifest.c defossilizes the name before the ordering compare. Because "\s" begins 0x5C and a space is 0x20, the two forms order oppositely, so a run like

T *sym-a\sb aaaa... T *sym-a-c bbbb...

is valid, fossil accepts it, and we rejected it. Branch names may contain spaces, so this is an ordinary tag, not a corner case -- the same post-decode trap already handled for F. tagOrderKey now builds the decoded key and both the parser check and the marshaller's sort call it, so the two cannot drift apart.

The Z card accepted any payload. Accepting a Z line inside the body is right -- §4.5.1 puts Z in neither occurrence class and treats an earlier Z as content covered by the final checksum -- but §4.7.19 still fixes the payload at exactly 32 hexadecimal characters. Validating nothing left an arbitrary-byte channel in an otherwise fully typed grammar and would have stored and forwarded artifacts canonical refuses. Upper-case digits are accepted, matching fossil's validate16.

Also: the T sign prefix is now checked against {+,-,*} per §4.7.16, both because canonical checks it and because an unvalidated sign byte >= 0x80 would UTF-8-expand to two bytes when the ordering key is assembled and silently corrupt the comparison. Two stale §4.7.20 citations corrected to §4.7.16, and the two-level T test gained the prefix-name case that actually distinguishes a two-level comparison from a concatenated one.

* Cite the normative grammar for the case-insensitive Z token

The Z payload's case-insensitivity was justified by inferring it from the word "hexadecimal" in §4.7.19. §6.1 states it normatively: md5-token = 32hexdig-ci, a parse-accept grammar admitting both 0-9a-f and A-F. Cite that instead, and record the split it implies -- the structural check is case-insensitive while §6.2 keeps verification byte-exact against the lowercase form.

Also soften deck.Compare's claim that the delta-manifest merge walk relies on F ordering to the future tense parse.go already uses: the merge walk is a map overlay today with no comparator to share. check-in: 5036cae392 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