Skip to content
go-libfossil Check-in [1a33f22119]
Login

Check-in [1a33f22119]

Overview
Comment:fix: remove StatusOpts, MergeOpts, CheckoutOpts — no call sites (#97)

* 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.

Timelines: family | ancestors | descendants | both | trunk
Files: files | file ages | folders
SHA3-256: 1a33f2211906ceec84fb4368a9719739632ef9a6b73de8061dee685a7faa1a88
User & Date: [email protected] on 2026-07-19 23:16:12.000
Other Links: manifest | tags
Context
2026-07-19
23:33
Add libfossil version command (#99)

* 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

23:16
fix: remove StatusOpts, MergeOpts, CheckoutOpts — no call sites (#97)

* 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

22:42
fix: clone_seqno is server-to-client only (#96)

* 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

Changes